A few fixes to improve MMFF/UFF robustness

[ptosco]

• fixed a bug in Code/ForceField/MMFF/testMMFFForceField.cpp
• fixed a bug in Code/GraphMol/ForceFieldHelpers/MMFF/AtomTyper.cpp
  which caused misassignment of atom types in CYGUAN01 upon shuffling
  the order of atoms in the validation SDF files
• changed hard-coded constants into const
• added checks for acos and asin function parameters to be within
  a (-1, 1) range

[greglandrum]

Question: is it better to set the values to (-1.0,1.0) or to raise some kind of value error.
I'm not sure how often this occurs or under what circumstances. If it's a round-off correction thing, then this makes sense, but if it's more to hammer things that are really out-of-range due to other problems back in range so that you don't get NaNs from other functions, this feels wrong to me.

[ptosco]

Hi Greg,

I don't think this should ever occur, given the way cosine and sine functions are computed. I think the only possibility are small round off issues (if ever) for angles close to 0, 180 degrees for cosine and 90,270 for sine. This was one of the concerns of the reviewer, I don't think it slows down the code significantly, so I decided to address it.

Cheers,
p.

Paolo Tosco, Ph.D.
Department of Drug Science and Technology
Via Pietro Giuria, 9 - 10125 Torino (Italy)
Tel: +39 011 670 7680 | Mob: +39 348 5537206
Fax: +39 011 670 7687 | E-mail: paolo.tosco@unito.it

http://open3dqsar.org | http://open3dalign.org

On 2 Jun 2014, at 07:08, Greg Landrum notifications@github.com wrote:

In Code/ForceField/MMFF/Params.h:

@@ -40,6 +40,14 @@ namespace ForceFields {
inline bool isDoubleZero(const double x) {
return ((x < 1.0e-10) && (x > -1.0e-10));
}

• inline void clipToOne(double &x) {
  Question: is it better to set the values to (-1.0,1.0) or to raise some kind of value error.
  I'm not sure how often this occurs or under what circumstances. If it's a round-off correction thing, then this makes sense, but if it's more to hammer things that are really out-of-range due to other problems back in range so that you don't get NaNs from other functions, this feels wrong to me.

—
Reply to this email directly or view it on GitHub.