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 e08e0d16d (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.
* change minimal cmake version to a consistent 3.5
* progress towards a cleanup
* get the basic python deps working
* two more libs
* another round of changes
all tests pass at this point
* next round of changes
all tests pass at this point
* close to done
all tests pass
* very close
* almost done
* shift the RDBoost dependencies around a bit
* remove an extraneous python linkage
this is trying to get the mac builds working again
* Only link to python if it was built shared (#3091)
* change in response to review
Co-Authored-By: Ric <ricrogz@users.noreply.github.com>
* move that suppression of the maybe-uninitialized warning to BoostStartInclude.h
Co-authored-by: Brian Kelley <fustigator@gmail.com>
Co-authored-by: Ric <ricrogz@users.noreply.github.com>
* disable builds of the StructChecker code by default
* operator"" _smarts() doesn't need to catch sanitization errors
* remove unused function
* turn back on some tests that shouldn't have been disabled
* Remove unused code from SMARTS parser and simplify a bit
SmilesParseOps::AddFragToMol is now used only from the SMARTS parser, so we can simplify the API
* Removes obsolete special case code for SMARTS
This was relevant when organic atoms in SMARTS queries were stored as two-part queries.
* improve SMARTS testing
make sure we can generate SMARTS from all the examples and then parse that again.
* Fixes#2814
* Fixes#2815
* some additional smarts tests to improve coverage
* test copy ctor and getPos
* remove obsolete test_list files
* include tests for the morgan invariant generators
* more cleanups and coverage improvements
* remove files that were mistakenly added
a vector of shared_ptr to Snapshots
- The PySnapshot class was removed
- the Trajectory::readAmber and Trajectory::readGromos member functions
were converted into non-member functions
- tests were modified accordingly
the horrible cross-library exception handling mess on linux. This may well break things on windows, which
might want these things static. Regardless, even as it is, this should be considered experimental
