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

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

Please also see the few additional points I raised.

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-11T14:52:50Z

+        """
+        Compute RMSD between this structure and *other* (no rotational alignment).
+
+        Both structures are translated to their centroid before comparison.


I would be a bit more precise here:
The two structure objects themselves are not modified. Please mention this.

If the objects themselves were changed, there could be side-effects in any code coming after calling this method. This is important to avoid!

timmyte · 2026-06-11T14:53:48Z

+        """
+        Compute RMSD between this structure and *other* using the Kabsch algorithm.
+
+        Translates both structures to their centroid, then finds the optimal


Same here. Mention that the Structure object themselves are not modified.

timmyte · 2026-06-11T15:00:08Z

+    # ------------------------------------------------------------------ #
+    def _resolve_masses(
+        self,
+        elem_masses: dict[str, float] | None = None,


Suggested change

elem_masses: dict[str, float] | None = None,

elem_masses: dict[str | Element, float] | None = None,

Also update the docstring below, please.

timmyte · 2026-06-11T15:00:23Z

+
+    def calc_moments_of_inertia(
+        self,
+        elem_masses: dict[str, float] | None = None,


Suggested change

elem_masses: dict[str, float] | None = None,

elem_masses: dict[str | Element, float] | None = None,

Also update the docstring below, please.

timmyte · 2026-06-11T15:01:14Z

+            `None` the moments are computed via
+            :meth:`calc_moments_of_inertia` using *mass_kwargs*.
+        **mass_kwargs
+            Forwarded to :meth:`calc_moments_of_inertia` when *moments* is


Suggested change

Forwarded to :meth:`calc_moments_of_inertia` when *moments* is

Forwarded to :meth:`calc_moments_of_inertia()` when `moments` is

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:35:08Z

+
+class TestGetCoordinates:
+    def test_shape(self, water):
+        coords = water.get_coordinates()


Please add a 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.

timmyte · 2026-06-11T15:38:26Z

+
+class TestSetCoordinates:
+    def test_mutates_in_place(self, water):
+        """set_coordinates should modify the structure in place."""


Suggested change

"""set_coordinates should modify the structure in place."""

"""set_coordinates should modify the structure in-place."""

timmyte · 2026-06-11T15:42:16Z

+        pm = water.calc_moments_of_inertia()
+        assert pm is not None
+        s = str(pm)
+        assert "Moments of inertia (amu·Å²):" in s


You should know the extract string that comes out.
Why not compare directly against that?

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

	elem_masses: dict[str, float] \| None = None,
	elem_masses: dict[str \| Element, float] \| None = None,

	Forwarded to :meth:`calc_moments_of_inertia` when moments is
	Forwarded to :meth:`calc_moments_of_inertia()` when `moments` is

	"""set_coordinates should modify the structure in place."""
	"""set_coordinates should modify the structure in-place."""

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!

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!

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!

timmyte left a comment

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!

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!

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!

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

crjacinto commented Apr 17, 2026 •

edited

Loading