RMSD and Rotational Constants Modules by crjacinto · Pull Request #230 · faccts/opi

crjacinto · 2026-04-17T12:13:28Z

Closes Issues

Description

Class methods to calculate the RMSD between two structures and the rotational spectrum (rotor type, moments of inertia and rotational constants) of a molecule.

Release Notes

(Project adheres to Keep a Changelog; Every entry should start in upper-case and end with (#<pr-id>); Remove sections that don't apply)

Added

Added constants.py containing some universal constants
Added rotconst.py containing auxiliary dictionaries for Rotational Constants Calculation

Changed

Modified structure.py with RMSD and RotConst as Class methods
Modified elements.py adding atomic masses for moment of inertia calculations

Removed

Removed redundant rmsd.py module

timmyte

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.

timmyte · 2026-04-22T14:24:47Z

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

Very nice improvement of the code. We are getting there.
I really appreciate how well you documented the code.

timmyte · 2026-05-06T20:48:03Z

+        Only ``Atom`` instances contribute; ``GhostAtom``, ``PointCharge``,
+        and ``EmbeddingPotential`` atoms are silently ignored.
+
+        Mass priority


This block should come after Parameters. This applies to every docstring that has this.

First explain your terminology, then you can explain the rules.

Change made, thanks for the explanation

Did you forget to push the change?

Please move the block after Parameters.

timmyte

Thanks for tackling my remarks. There are still some unresolved issues though.

Please also see the few additional points I raised.

timmyte · 2026-05-20T16:16:29Z

+            Coordinates in the same order as the input list.
+        """
+        atom_list = atoms if atoms is not None else self.atoms
+        return np.array([a.coordinates.coordinates for a in atom_list], dtype=np.float64)


We should refrain from setting dtype explicitly everywhere, as this introduces an extra chore, which might be overlooked by developers.

Or is there a particular reason, why we need to set this explicitly?

dtype is still set explicitly. Please remove this, if there is no particular reason to do so. Otherwise, add comment mentioning the reason.

timmyte · 2026-05-20T16:21:42Z

+        Only ``Atom`` instances contribute; ``GhostAtom``, ``PointCharge``,
+        and ``EmbeddingPotential`` atoms are silently ignored.
+
+        Mass priority


Did you forget to push the change?

timmyte

Looks good. Please consider my last remark before merging.
I also would like to get @haneug opinion on the API with all the different weight parameters

…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

Very good, especially the exhaustive amount of tests. BRAVO!

timmyte · 2026-06-11T15:34:53Z

+        """All atoms in water are real."""
+        assert len(water.real_atoms) == 3
+
+    def test_return_type(self, water):


Please add docstring, briefly describing the purpose of the test.

timmyte · 2026-06-11T15:37:31Z

+class TestGetCoordinatesAtCentroid:
+    def test_centroid_is_zero(self, water):
+        coords = water.get_coordinates_at_centroid()
+        np.testing.assert_allclose(coords.mean(axis=0), [0.0, 0.0, 0.0], atol=1e-12)


Please document why so picked this value for atol?
Please do this for all tests below.
It's not obvious why its 1e-12 here and 1e-6 below.

…ng unit tests.

…sd.py

timmyte

Thanks for quickly tackling my remarks. But please make sure you addressed all of them.

timmyte · 2026-06-17T16:59:02Z

+            Coordinates in the same order as the input list.
+        """
+        atom_list = atoms if atoms is not None else self.atoms
+        return np.array([a.coordinates.coordinates for a in atom_list], dtype=np.float64)


dtype is still set explicitly. Please remove this, if there is no particular reason to do so. Otherwise, add comment mentioning the reason.

timmyte · 2026-06-17T17:01:13Z

+        Only ``Atom`` instances contribute; ``GhostAtom``, ``PointCharge``,
+        and ``EmbeddingPotential`` atoms are silently ignored.
+
+        Mass priority


Please move the block after Parameters.

timmyte · 2026-06-17T17:02:49Z

@@ -1475,13 +1494,14 @@ def calc_rotational_constants(

        Mass priority


Same here: Please make that the Parameters block comes first and then the Mass priority

timmyte · 2026-06-17T17:03:20Z

    effectively zero (degenerate / linear axis).
    """
-    if inertia is None or inertia < 1e-6:
+    if inertia is None or inertia < 1e-3:


Why did this number change now!?
Please document why exactly 10e-3.

timmyte · 2026-06-17T17:05:42Z

    def test_rotated_zero_rmsd(self, water, water_rotated):
        """Kabsch should align the rotation and give ~0 RMSD."""
        centered = water.centered_structure()
+        print(centered.get_coordinates())


Why the print statement here?

crjacinto requested a review from a team as a code owner April 17, 2026 12:13

crjacinto changed the title ~~RMSD and Rotationl Constants Modules~~ WIP: RMSD and Rotationl Constants Modules Apr 17, 2026

crjacinto changed the title ~~WIP: RMSD and Rotationl Constants Modules~~ WIP: RMSD and Rotational Constants Modules Apr 17, 2026

crjacinto changed the title ~~WIP: RMSD and Rotational Constants Modules~~ RMSD and Rotational Constants Modules Apr 17, 2026

timmyte self-requested a review April 20, 2026 13:02

timmyte requested changes Apr 20, 2026

View reviewed changes

timmyte assigned timmyte and crjacinto and unassigned timmyte Apr 28, 2026

crjacinto requested a review from timmyte May 6, 2026 14:12

timmyte requested changes May 6, 2026

View reviewed changes

crjacinto requested a review from timmyte May 19, 2026 18:24

timmyte reviewed May 20, 2026

View reviewed changes

crjacinto requested a review from timmyte May 22, 2026 14:17

crjacinto force-pushed the feature-rmsd branch from bc4679a to 87bdce9 Compare May 27, 2026 12:32

timmyte approved these changes Jun 2, 2026

View reviewed changes

Comment thread src/opi/input/structures/structure.py

Comment thread src/opi/input/structures/structure.py Outdated

Comment thread src/opi/input/structures/structure.py Outdated

Comment thread src/opi/utils/rotconst.py Outdated

timmyte requested a review from haneug June 2, 2026 14:08

crjacinto added 12 commits June 10, 2026 14:54

RMSD and Rotationl Constants Modules

29e1915

Fixed missing type annotations

21df17b

Fixed error in documentation

02f9514

Fixed RMSD and RotConst implementation. Modified structure.py and ele…

a901cb2

…ment.py. Added units.py and rotconts.py

Removed rmsd.py external module

72c8277

Fixed typos and ambiguous variable names

3b3644c

Splitted RotationalConstatns() function in 3 to get separately Rot. C…

e688f54

…onstatns, Inertia Moments, and Rotor Type

Ran nox session

4c1c498

Fixed errors and added unit tests for RMSD and RotConst implementations

0ba7dc7

Corrected double ticks

bd8da57

Changed Atom lists to Indeces list. Added property for real_atoms

03ba66d

Updated test_rmsd to use new functions in structure.py

999b4af

crjacinto added 5 commits June 10, 2026 14:54

Fixed a typo

93d55d0

Ran nox session

45bdfd0

Modified set_coordinates() and centered_structure(). Added relative t…

32236ce

…olerance for rotor classification. Improved unit tests

Modified calc_rotataional_constants(), added private function _resolv…

723dfc9

…e_masses(), and updated unit tests

Modified mass priority: elem_masses > atom.mass > default (ATOMIC_MAS…

1c8fe24

…SES_FROM_ELEMENT)

crjacinto force-pushed the feature-rmsd branch from a50c944 to 1c8fe24 Compare June 10, 2026 13:01

crjacinto requested a review from timmyte June 10, 2026 13:03

timmyte requested changes Jun 11, 2026

View reviewed changes

crjacinto added 2 commits June 15, 2026 11:53

Modified structure.py modules: RMSD and RotConst. Updated correspondi…

293c7c8

…ng unit tests.

Fixed some typos on structure.py and added some docstrings in test_rm…

53b9c4b

…sd.py

crjacinto requested a review from timmyte June 15, 2026 10:21

crjacinto mentioned this pull request Jun 15, 2026

WIP: Added instance method to generate MolBar from Structure #236

Open

timmyte requested changes Jun 17, 2026

View reviewed changes

		@@ -1475,13 +1494,14 @@ def calc_rotational_constants(

		Mass priority

Uh oh!

Conversation

crjacinto commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Closes Issues

Description

Release Notes

Added

Changed

Removed

Uh oh!

timmyte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timmyte commented Apr 22, 2026

Uh oh!

timmyte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crjacinto May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

timmyte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

timmyte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timmyte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crjacinto commented Apr 17, 2026 •

edited

Loading

crjacinto May 19, 2026 •

edited

Loading