RMSD and Rotational Constants Modules #230
Conversation
timmyte
left a comment
There was a problem hiding this comment.
Solid first PR.
But the tools are bit to decoupled from OPI. You should use and add to the data structures that we already have in OPI.
|
As discussed, I would design the RMSD function as follows: class Structure:
def rmsd(self, other: Structure, /, only_atoms: Sequence[int] = (), *, ignore_hs: bool = False) -> float:
return _rmsd(self.coordinates, other.coordinates, only_atoms=only_atoms, ignore_hs=ignore_hs)
def _rmsd(coords1: NDArray[float], coords2: NDArray[float], /, only_atoms: Sequence[int] = (), *, ignore_hs: bool = False)
# We ignore `ignore_hs` if `only_atoms` is populated. This should also be documented
if coords1.shape != coords2.shape:
raise ValueError
def rmsd_kabsch(self, other: Structure, /, only_atoms: Sequence[int] = (), *, ignore_hs: bool = False) -> float:
# 1) Translate to center of geometry (centroid)
# 2) find optimal rotation
# `only_atoms` and `ignore_hs` don't need to be passed here anymore as `coords1` and `coords2_rotated` should only contain the coordinates of the relevant atoms.
return _rmsd(coords1, coords2_rotated) |
timmyte
left a comment
There was a problem hiding this comment.
Very nice improvement of the code. We are getting there.
I really appreciate how well you documented the code.
timmyte
left a comment
There was a problem hiding this comment.
Thanks for tackling my remarks. There are still some unresolved issues though.
Please also see the few additional points I raised.
…ment.py. Added units.py and rotconts.py
…onstatns, Inertia Moments, and Rotor Type
…olerance for rotor classification. Improved unit tests
…e_masses(), and updated unit tests
…SES_FROM_ELEMENT)
timmyte
left a comment
There was a problem hiding this comment.
Very good, especially the exhaustive amount of tests. BRAVO!
timmyte
left a comment
There was a problem hiding this comment.
Thanks for quickly tackling my remarks. But please make sure you addressed all of them.
There was a problem hiding this comment.
Please address all of the comments by Tim and resolve them when they are resolved. I added some more. Also in PR message on its own you should only add to the section Added and do not describe stuff that changed within the PR.
Edit: If they are just regular comments and you cannot resolve them thumps up also works for me to see that you addressed them.
…d speed of light units. Cleaned up unit tests for RMSD and RotConst. Modifed changelog
haneug
left a comment
There was a problem hiding this comment.
Really nice PR! I just made some small style changes and will merge this now.
Closes Issues
Closes #220 #219
Description
Release Notes
Added
rmsdandrmsd_kabschto calculate RMSD without and with rotational alignment toStructureclass (RMSD and Rotational Constants Modules #230).calc_rotational_constantsto calculate molecular rotational constants toStructureclass (RMSD and Rotational Constants Modules #230).calc_rotor_typeto classify a molecule's rotor type toStructureclass (RMSD and Rotational Constants Modules #230).