From 443fdf1adba52453bb0004852c6c3a3beec24f77 Mon Sep 17 00:00:00 2001 From: Clay Moore Date: Thu, 28 May 2026 07:55:50 -0500 Subject: [PATCH] Gate BFGS gradient-convergence denominator fix on RDKit >= 2026.03 rdkit/rdkit#9298 (merged into RDKit 2026.03) fixed a bug where a negative energy value caused max(energy * gradScale, 1.0) to clamp to 1, artificially tightening the gradient-tolerance convergence test mid-minimisation. Force fields with stabilising electrostatic or dispersion terms (MMFF94, UFF) can produce negative intermediate energies, so this affected real workloads. Apply the same fix in both nvMolKit BFGS paths, gated on the linked RDKit version so that behaviour is unchanged when built against older RDKit: - src/minimizer/bfgs_minimize.cu (updateDGradKernel, batched path) - src/minimizer/bfgs_minimize_permol_kernels.cu (updateDGrad, per-mol path) Mirrors the version-conditional pattern already used for kRdkitHasGradScaleFix in scaleGradKernel. Signed-off-by: Clay Moore --- src/minimizer/bfgs_minimize.cu | 12 +++++++----- src/minimizer/bfgs_minimize_permol_kernels.cu | 7 ++++++- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/minimizer/bfgs_minimize.cu b/src/minimizer/bfgs_minimize.cu index e240eecd..b40cfa1c 100644 --- a/src/minimizer/bfgs_minimize.cu +++ b/src/minimizer/bfgs_minimize.cu @@ -908,11 +908,13 @@ __global__ void updateDGradKernel(const double gradTol, double blockMax = cub::BlockReduce(tempStorage).Reduce(localMax, cubMax()); if (idxWithinSystem == 0) { - // Matches RDKit's signed-energy convergence denominator in ForceField::minimize. - // Negative-energy geometries can clamp this to 1, artificially tightening the - // check — but fixing it unconditionally diverges from reference behavior. - // TODO: file upstream RDKit bug; gate fix on RDKit version once merged there. - const double term = max(energies[sysIdx] * gradScales[sysIdx], 1.0); + // rdkit/rdkit#9298 (merged RDKit 2026.03) fixed the signed-energy denominator bug: + // raw negative energy clamped the denominator to 1, artificially tightening gradTol. + // Use |energy| when linked against a fixed RDKit; keep signed otherwise for parity. + constexpr bool kRdkitHasGradDenomFix = + RDKIT_VERSION_MAJOR > 2026 || (RDKIT_VERSION_MAJOR == 2026 && RDKIT_VERSION_MINOR >= 3); + const double energyMag = kRdkitHasGradDenomFix ? fabs(energies[sysIdx]) : energies[sysIdx]; + const double term = max(energyMag * gradScales[sysIdx], 1.0); blockMax /= term; if (blockMax < gradTol) { // Converged diff --git a/src/minimizer/bfgs_minimize_permol_kernels.cu b/src/minimizer/bfgs_minimize_permol_kernels.cu index cdd463cb..ee4b77d7 100644 --- a/src/minimizer/bfgs_minimize_permol_kernels.cu +++ b/src/minimizer/bfgs_minimize_permol_kernels.cu @@ -292,7 +292,12 @@ __device__ void updateDGrad(const int float blockMax = cub::BlockReduce(tempStorage).Reduce(localMax, cubMax()); if (threadIdx.x == 0) { - const float term = max(energy * gradScale, 1.0); + // rdkit/rdkit#9298 (merged RDKit 2026.03): use |energy| to avoid clamping the + // denominator to 1 when energy is negative; match signed behavior on older RDKit. + constexpr bool kRdkitHasGradDenomFix = + RDKIT_VERSION_MAJOR > 2026 || (RDKIT_VERSION_MAJOR == 2026 && RDKIT_VERSION_MINOR >= 3); + const double energyMag = kRdkitHasGradDenomFix ? fabs(energy) : energy; + const float term = max(energyMag * gradScale, 1.0); blockMax /= term; if (blockMax < gradTol) { converged = true;