Cleanup: Force field #7406
Cleanup: Force field #7406
Conversation
There was a problem hiding this comment.
I made a few suggestions here.
There is a CI failure in the tests on intel-based Macs that looks real.
I was unsuccessful at getting an x64 build working on my Mac this weekend, so I couldn't track down what was causing this.
I would be interested to see if the problem goes away if you revert the changes that I flagged as having potential numerical instabilities. If that doesn't work then I will go back to trying to the x64 env working on my Mac.
Co-authored-by: Greg Landrum <greg.landrum@gmail.com>
Code/ForceField/UFF/TorsionAngle.cpp
Outdated
| // cos(3x) = cos^3(x) - 3*cos(x)*sin^2(x) | ||
| cosNPhi = cosPhi * (cosPhi * cosPhi - 3. * sinPhiSq); | ||
| // cos(3x) = cos^3(x) - 3*cos(x)*sin^2(x) = 4cos^3(x) -3cos(x) | ||
| cosNPhi = cosPhi * (4 * cosPhi * cosPhi - 3.); |
There was a problem hiding this comment.
I got an x64 (intel) build working on my M1 Mac and can now reproduce the problems observed in the CI builds.
This change is responsible for the failure: the minimization at line 163 of Code/ForceField/Wrap/testConstraints.py fails to converge in the default 200 iterations. If I bump the number of itrations up to 211, then it does pass.
When using the original formulation - cosNPhi = cosPhi * (cosPhi * cosPhi - 3. * sinPhiSq) - the minimization converges in ~150 iterations.
Subtracting a constant from these cosPhi products definitely does change something with the stability of the numerics.
Change is not necessarily bad: it could be that your version is actually more stable and that it's highlighting what was previously a problem that we had not noticed.
Your comment on the commits that make this change indicate that you were intending to improve the stability of the numerics; do you have some kind off references indicating that the way you restructured these calculations is actually more stable?
|
Thanks for another nice set of cleanups @AnnaBruenisholz ! |
| @@ -278,22 +263,17 @@ class RDKIT_FORCEFIELD_EXPORT MMFFChgCollection { | |||
| sign = 1; | |||
| } | |||
| #ifdef RDKIT_MMFF_PARAMS_USE_STD_MAP | |||
| std::map<const unsigned int, | |||
| std::map<const unsigned int, MMFFChg>>::const_iterator res1; | |||
| std::map < const unsigned int, | |||
There was a problem hiding this comment.
hm, this line is not right, I think it should have been deleted.
| unsigned int temp = canIAtomType; | ||
| canIAtomType = canKAtomType; | ||
| canKAtomType = temp; |
There was a problem hiding this comment.
This could have been a std::swap(canIAtomType, canKAtomType);
| unsigned int temp = canIAtomType; | ||
| canIAtomType = canKAtomType; | ||
| canKAtomType = temp; |
| for (RDKit::SnapshotVect::const_iterator it = snapshotVect.begin(); | ||
| it != snapshotVect.end(); ++it) { | ||
| l.append(new RDKit::Snapshot(*it)); | ||
| for (auto it : snapshotVect) { |
There was a problem hiding this comment.
Should this be auto& it, to avoid an extra copy ?
If only you'd been 2 hours quicker! ;-) Thanks for catching the problems. I'll fix them in a separate PR |
What does this implement/fix? Explain your changes.
Some minor changes to modernize Code/ForceField.