Optionally include hydrogens in RMSDs #143
Conversation
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
|
This is updated and ready for review, though I'm not sure how heavy a priority it is |
| ) | ||
| for key in qm_points | ||
| ) | ||
| / len(qm_points), |
There was a problem hiding this comment.
Note this detail - without which there were numeric differences, i.e. 24x for a scan of 24 grid points
There was a problem hiding this comment.
Just noting that RMSD.from_data computes the RMS RMSD as opposed to the mean. Computing the mean is probably a better way to do it, but maybe RMSD.from_data should also be changed to use the mean? Should RMSD.from_data be used (and modified to have an include_hydrogens kwarg) instead of duplicating (a tiny bit)? Thanks.
There was a problem hiding this comment.
Good catch! Would the same suggestion also apply to RMSE.from_data?
yammbs/yammbs/torsion/analysis.py
Line 122 in a31957d
There was a problem hiding this comment.
Since this is not strictly related to the intent of the PR I'm going to split it out into a separate issue: #186
j-wags
left a comment
j-wags
left a comment
There was a problem hiding this comment.
I see this was demoted to draft - Dropping my one comment from my partial review for now. Happy to be assigned to review again once this is ready.
| RMSD( | ||
| qcarchive_id=id, | ||
| rmsd=get_rmsd(molecule, qm, mm), | ||
| rmsd=get_rmsd( |
There was a problem hiding this comment.
(not blocking) It's quite confusing that get_rmsd is the name of both a function and a method (with different purposes/signatures) - could be a good thing to clean up/disambiguate in the code
|
There are some moving pieces in RMSD analysis; I'm tabling this to avoid diverging branches |
Closes #141
This code path is completely dependent on OpenEye. How has this not been a problem before?