Fix BFGS gradient-convergence denominator for negative energies by mooreneural · Pull Request #9298 · rdkit/rdkit (original) (raw)
In Code/Numerics/Optimizer/BFGSOpt.h, the gradient-convergence check computed
double term = std::max(funcVal * gradScale, 1.0);
...
test /= term;
if (test < gradTol) return 0;When funcVal (the current energy) is negative, funcVal * gradScale is negative and std::max clamps the denominator to 1.0. The convergence test therefore divides the gradient norm by 1 instead of by the intended |E| * gradScale, which over-tightens the criterion by a factor of |funcVal * gradScale| whenever |funcVal * gradScale| > 1.
Negative energies are a normal mid-minimization state for force fields that include stabilizing terms (MMFF94, UFF with charges, AMBER-style potentials), so this affects realistic workloads: extra BFGS iterations or, occasionally, hitting MAXITS and returning the "too many iterations" status when convergence would otherwise have been reached.
The fix is to use |funcVal| in the denominator, matching the pattern used three lines below ('std::max(fabs(pos[i]), 1.0)') and matching the intended interpretation as a magnitude.
A new test case 'testBFGSOptimizationNegativeEnergy' in testOptimizer.cpp minimizes a 2D quadratic whose value is always negative along the convergence path and verifies the optimizer reaches the analytic minimum.
git blame attributes the original line to commit e08e0d1 (Nov 2015), when the optimizer was restructured; the surrounding code does use absolute values, so this reads as an oversight rather than an intentional choice.
6 tasks
scal444 pushed a commit to NVIDIA-BioNeMo/nvMolKit that referenced this pull request
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 claytonwaynemoore@gmail.com
greglandrum pushed a commit that referenced this pull request
In Code/Numerics/Optimizer/BFGSOpt.h, the gradient-convergence check computed
double term = std::max(funcVal * gradScale, 1.0);
...
test /= term;
if (test < gradTol) return 0;When funcVal (the current energy) is negative, funcVal * gradScale is negative and std::max clamps the denominator to 1.0. The convergence test therefore divides the gradient norm by 1 instead of by the intended |E| * gradScale, which over-tightens the criterion by a factor of |funcVal * gradScale| whenever |funcVal * gradScale| > 1.
Negative energies are a normal mid-minimization state for force fields that include stabilizing terms (MMFF94, UFF with charges, AMBER-style potentials), so this affects realistic workloads: extra BFGS iterations or, occasionally, hitting MAXITS and returning the "too many iterations" status when convergence would otherwise have been reached.
The fix is to use |funcVal| in the denominator, matching the pattern used three lines below ('std::max(fabs(pos[i]), 1.0)') and matching the intended interpretation as a magnitude.
A new test case 'testBFGSOptimizationNegativeEnergy' in testOptimizer.cpp minimizes a 2D quadratic whose value is always negative along the convergence path and verifies the optimizer reaches the analytic minimum.
git blame attributes the original line to commit e08e0d1 (Nov 2015), when the optimizer was restructured; the surrounding code does use absolute values, so this reads as an oversight rather than an intentional choice.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})