Fix multi chain RMSD analysis by hannahbaumann · Pull Request #64 · OpenFreeEnergy/openfe_analysis

hannahbaumann · 2025-12-18T12:48:36Z

Fixes #30

hannahbaumann · 2026-01-06T09:38:33Z

Check if this works for triclinic boxes. From Irfan: There might also be code in MDAnalysis that does the math also working for triclinic boxes.

…e runners for the matrix

for more information, see https://pre-commit.ci

src/openfe_analysis/rmsd.py

hannahbaumann · 2026-02-06T16:16:43Z

pre-commit.ci autofix

src/openfe_analysis/tests/test_reader.py

IAlibay

Couple of small things - main question is about the fractional to cartesian return, should that be on box or box.T?

src/openfe_analysis/rmsd.py

src/openfe_analysis/transformations.py

IAlibay · 2026-02-15T00:21:57Z

src/openfe_analysis/transformations.py

-
-            # this only works for orthogonal boxes
-            ag.positions -= np.rint(vec / box) * box
+            frac = np.linalg.solve(box.T, vec)  # fractional coordinates


This seems to make sense.

However I'm not 100% sure about the shift calculation below, do you have a reference for this / where it was inspired from?

This was inspired from the wrap_positions function here:
https://gitlab.com/ase/ase/-/blob/master/ase/geometry/geometry.py

IAlibay · 2026-02-15T12:59:22Z

src/openfe_analysis/tests/test_rmsd.py

    assert_allclose(
        output["protein_RMSD"][0][:6],
-        [0, 30.620948, 31.158894, 1.045068, 30.735975, 30.999849],
+        [0, 1.089747, 1.006143, 1.045068, 1.476353, 1.332893],


Were these hand calculated or just a regression test for how the numbers changed?

This is just a regression test for how the numbers changed.

Ok - not here but maybe when you move things to separate classes, it might be good to have at least one test where you hand verify the accuracy. I find that this can be important in sanity checking things.

src/openfe_analysis/rmsd.py

src/openfe_analysis/transformations.py

src/openfe_analysis/rmsd.py

src/openfe_analysis/transformations.py

IAlibay · 2026-02-15T14:38:05Z

src/openfe_analysis/transformations.py

-            # this only works for orthogonal boxes
-            ag.positions -= np.rint(vec / box) * box
+            frac = np.linalg.solve(box.T, vec)  # fractional coordinates
+            shift = np.dot(np.round(frac), box)  # nearest image, then compute shift


Looking at https://www.homepages.ucl.ac.uk/~rmhajc0/frorth.pdf, should this not be applied to box.T here? (equation 17)

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

hannahbaumann · 2026-02-16T14:30:20Z

I realized that the test for the ClosestImageShift class was on a frame that didn't need shifting, so I updated the test to first shift the ligand arbitrarily into a different image and then to see whether we get it back to the same COM distance as before.

IAlibay

This looks good to me! Let's see how it goes in practice :)

Fix stab at fixing multi chain RMSD analysis

4b257b2

hannahbaumann changed the title ~~Fix multi chain RMSD analysis~~ [WIP] Fix multi chain RMSD analysis Dec 18, 2025

hannahbaumann added 4 commits December 18, 2025 14:56

Some updates

236ff72

Add tests

d274b0c

Some fixes

f3634dd

Add another test

e92adb3

hannahbaumann linked an issue Dec 19, 2025 that may be closed by this pull request

Analysis does not work properly for proteins with multiple chains #30

Closed

hannahbaumann self-assigned this Dec 19, 2025

hannahbaumann marked this pull request as ready for review January 16, 2026 10:30

hannahbaumann and others added 20 commits January 16, 2026 15:57

Move some tests to use skipped smaller data

b528ca2

Test out zenodo dealings

a477bc1

Try to improbe speed

ad84082

Try removing locking

8ba8087

Run downloads before the testing to have a single download for all th…

ead7951

…e runners for the matrix

add import pooch

f898a35

Test out more

c675a5c

Ensure datasets get closed

88e456d

Move to per test download again

73a8e4d

Remove commented out lines

43aaca2

Test out adding an extra slash

c165525

Switch to all version doi

5f17770

Download url directly

c28286e

Small fix

197b6ba

Change url

b45390a

Add missing s

1d70936

Switch to api url

20084c3

Revert to old cli

1a1c916

Update cli.py

59c7392

Update cli.py

5e135ab

hannahbaumann and others added 2 commits February 2, 2026 09:02

Apply suggestion from @hannahbaumann

6c1a6c5

Remove unnecessary make_whole

f4637f4

IAlibay self-assigned this Feb 2, 2026

hannahbaumann and others added 2 commits February 3, 2026 10:03

Merge branch 'main' into fix_rmsd_multichain

e7d6935

[pre-commit.ci] auto fixes from pre-commit.com hooks

deb5126

for more information, see https://pre-commit.ci

hannahbaumann commented Feb 3, 2026

View reviewed changes

src/openfe_analysis/rmsd.py Outdated Show resolved Hide resolved

jameseastwood unassigned jthorton Feb 4, 2026

hannahbaumann and others added 3 commits February 6, 2026 15:54

Merge branch 'main' into fix_rmsd_multichain

fa3227e

Small fix

43eb039

Merge branch 'main' into fix_rmsd_multichain

73fe2ee

Update tests

7be4c53

hannahbaumann requested a review from jthorton February 10, 2026 08:03

hannahbaumann commented Feb 10, 2026

View reviewed changes

src/openfe_analysis/tests/test_reader.py Outdated Show resolved Hide resolved

Apply suggestion from @hannahbaumann

68a2aab

IAlibay mentioned this pull request Feb 13, 2026

Refactor reader.py #77

Merged

IAlibay requested changes Feb 15, 2026

View reviewed changes

hannahbaumann and others added 7 commits February 16, 2026 13:01

Update src/openfe_analysis/transformations.py

198156b

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

Update src/openfe_analysis/transformations.py

6f85466

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

Update src/openfe_analysis/transformations.py

1d19473

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

Update src/openfe_analysis/rmsd.py

6cb52af

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

Update src/openfe_analysis/rmsd.py

2ccc7a8

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

Modify test for closest image shift

24163e5

Small fix

5994774

hannahbaumann added 2 commits February 16, 2026 15:50

Alternate ClosestImageShift

d2b2c34

Add caveat to doc string

592149c

IAlibay approved these changes Feb 16, 2026

View reviewed changes

hannahbaumann merged commit d67cfc2 into main Feb 16, 2026
7 checks passed

hannahbaumann deleted the fix_rmsd_multichain branch February 16, 2026 15:57

Conversation

hannahbaumann commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hannahbaumann commented Jan 6, 2026

Uh oh!

Uh oh!

hannahbaumann commented Feb 6, 2026

Uh oh!

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IAlibay Feb 15, 2026

Choose a reason for hiding this comment

Uh oh!

hannahbaumann Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

IAlibay Feb 15, 2026

Choose a reason for hiding this comment

Uh oh!

hannahbaumann Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

IAlibay Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IAlibay Feb 15, 2026

Choose a reason for hiding this comment

Uh oh!

hannahbaumann commented Feb 16, 2026

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hannahbaumann commented Dec 18, 2025 •

edited

Loading