Fix: Incorrect `capped_distance` in some triclinic box situations by BradyAJohnston · Pull Request #5481 · MDAnalysis/mdanalysis

BradyAJohnston · 2026-10-09T11:07:44Z

In triclinic boxes where both b_x and c_y are non-zero (e.g. truncated octahedra), the nsgrid and pkdtree methods of capped_distance / self_capped_distance miss pairs within the cutoff. This affects bond guessing, selections, etc. Reproduced with the issue's struc.txt and the truncated-octahedron script; both now match bruteforce.

Changes made in this Pull Request:

_triclinic_pbc: fix the a-axis wrapping bound, which could leave coordinates just outside the primary image (affects apply_PBC, pkdtree and nsgrid)
FastNS.coord2cellxyz: fix the same missing term in the x cell index (as suspected here)
FastNS._prepare_box: size grid cells from the box's perpendicular widths, so cells are never thinner than the cutoff
Add regression tests for apply_PBC, capped_distance, self_capped_distance and FastNS in skewed boxes

LLM / AI generated code disclosure

LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: no

I did use Claude to scan through and track down the issue, but I wrote the code and the tests.

PR Checklist

Issue raised/referenced?
Tests updated/added?
Documentation updated/added?
package/CHANGELOG file updated?
Is your name in package/AUTHORS? (If it is not, add it!)
LLM/AI disclosure was updated.

Developers Certificate of Origin

I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.

read-the-docs-community · 2026-10-09T11:13:03Z

Documentation build overview

📚 MDAnalysis | 🛠️ Build #35238561 | 📁 Comparing dfb5613 against latest (6ae1589)

🔍 Preview build

17 files changed · ± 17 modified

± Modified

BradyAJohnston · 2026-10-09T11:21:53Z

Seems one of the old snapshots is failing but I'm pretty sure they are just incorrectly snapshotted values, looking into it

codecov · 2026-10-09T11:35:48Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.91%. Comparing base (4ba8fe7) to head (dfb5613).

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #5481   +/-   ##
========================================
  Coverage    93.91%   93.91%           
========================================
  Files          184      184           
  Lines        22644    22644           
  Branches      3219     3219           
========================================
+ Hits         21266    21267    +1     
  Misses         917      917           
+ Partials       461      460    -1

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

IAlibay · 2026-10-09T11:41:49Z

This looks great, unfortunately I won't have time to review. I'm going to un-request my review here and pick on someone else ;)

orbeckst · 2026-10-12T19:51:26Z

@IAlibay can we get this fix into the next release? It looks important enough as it can affect scientific correctness across many different quantities.

orbeckst · 2026-10-12T19:51:45Z

(Yes, I get that it needs to be reviewed...)

IAlibay · 2026-10-13T05:25:31Z

@IAlibay can we get this fix into the next release? It looks important enough as it can affect scientific correctness across many different quantities.

Yes, as long as it can get merged by Sunday.

orbeckst · 2026-10-14T16:53:08Z

At first glance this looks very good, but I do need a bit more time to dig into it.

IAlibay · 2026-10-14T18:20:46Z

At first glance this looks very good, but I do need a bit more time to dig into it.

If "by Sunday" isn't possible - could you propose an alternative deadline? I do agree that this one probably falls under "critical fixes", so we could delay the release a tiny bit.

orbeckst · 2026-10-14T21:41:16Z

I'll try to keep the "by Sunday" deadline for this one. Primarily I have to convince myself that what the LLMs are telling me makes sense to me, too. It's been a while since I dug into PBC handling. I even looked up the original algorithm in Frenkel & Smit again – the classics, in FORTRAN!

BradyAJohnston · 2026-10-15T10:06:08Z

I've just made some tweaks, there was a 2x slowdown in FastNS construction from the additional numpy calls, but that's now been reduced to the same speeds. Tweaked the new tests a bit to clean them up.

…nston/mdanalysis into fix-4906-triclinic-pbc

- extensive CHANGELOG entry - versionchanged (because the fix is important) - code docs/comment updates

orbeckst

It took me a while to understand the fixes. Both make sense to me.

I added more docs (and additional test).

Overall I am happy with it and think it definitely needs to go into 2.11.0.

orbeckst · 2026-10-19T08:23:55Z

 * 2.11.0

 Fixes
+ * Fixed PBC handling in triclinic boxes in `_triclinic_pbc()` (used by


I added an extensive entry because the impact of the fix is potentially big as it can directly affect computed properties. I know that we normally keep this short but in this case I'd like to be pretty clear.

orbeckst · 2026-10-19T08:24:53Z

+   - `capped_distance()` and `self_capped_distance()` with the `nsgrid` or
+     `pkdtree` methods (and hence bond guessing, distance-based selections)
+     could miss pairs within the cutoff; `nsgrid` was affected in triclinic
+     boxes with two or more non-90º angles (e.g. rhombic dodecahedron),


Note that the patch contains multiple fixes and in fact affected more box shapes than originally noted.

orbeckst · 2026-10-19T08:27:06Z

            if (!msr) {
                // translate remainder of crd to central cell along a-axis
-                lbound = crd[1] * a_ax_yfactor + crd[2] * a_ax_zfactor;
+                lbound = -(crd[1] * bi3 + crd[2] * bi6) * box[0];


yes, this fix makes sense — I looked at it for a long time with diagrams, algebra, and LLM help, and I concur that this is necessary

orbeckst · 2026-10-19T08:30:32Z

+        # Cells must be at least `cutoff` thick perpendicular to each pair of
+        # box faces, so size the grid by the box's perpendicular widths.
+        # The box matrix is lower triangular, so width = volume / face area
+        # has a closed form.
+        ax = self.triclinic_dimensions[XX]
+        bx = self.triclinic_dimensions[XY]
+        by = self.triclinic_dimensions[YY]
+        cx = self.triclinic_dimensions[XZ]
+        cy = self.triclinic_dimensions[YZ]
+        cz = self.triclinic_dimensions[ZZ]
+        volume = ax * by * cz
+        widths[0] = widths[1] = widths[2] = 0.0
+        # invalid boxes give an all-zero matrix, leave widths at zero
+        if volume > 0:
+            widths[0] = volume / math.sqrt((by * cz) ** 2 + (bx * cz) ** 2
+                                           + (bx * cy - by * cx) ** 2)
+            widths[1] = by * cz / math.sqrt(cy * cy + cz * cz)
+            widths[2] = cz


This fixes issues with boxes that have more than one non-90º angle

orbeckst · 2026-10-19T08:32:49Z

+        xyz : numpy.ndarray int of shape (3,)
+            Cell coordinate for the given coordinate
+
+        Background


I think it's useful to write out how the calculation is done, given that the second key bug fix is the missing s_b b_x term

orbeckst · 2026-10-19T08:34:28Z


        eps = DielectricConstant(ag, make_whole=False).run()
-        assert_allclose(eps.results["eps_mean"], 721.711, rtol=1e-03)
+        # reference values depend on correct PBC handling


I wanted to make clear that this test is actually sensitive to PBC (which is good); the previous comment in this PR only said that the ref value changed but this historical info gets confusing over time without context

orbeckst · 2026-10-19T09:14:23Z

I quickly ran the new tests against the develop branch (without the fix).

The nsgrid test_issue_4906 test does not fail for the rhombic dodecahedron (box2).
All distance tests fail for all three example boxes.

$ pytest -v test_nsgrid_issue4906.py test_distances_issue4906.py
...
==================================================================== short test summary info =====================================================================
FAILED test_nsgrid_issue4906.py::test_issue_4906[box0] - AssertionError: assert {(np.int64(0)...64(325)), ...} == {(np.int64(0)...64(325)), ...}
FAILED test_nsgrid_issue4906.py::test_issue_4906[box1] - AssertionError: assert {(np.int64(0)...64(325)), ...} == {(np.int64(0)...64(325)), ...}
FAILED test_distances_issue4906.py::TestCappedDistances::test_capped_distance_skewed_triclinic[pkdtree-box0] - AssertionError: assert {(np.int64(0)...64(837)), ...} == {(np.int64(0)...64(837)), ...}
FAILED test_distances_issue4906.py::TestCappedDistances::test_capped_distance_skewed_triclinic[pkdtree-box1] - AssertionError: assert {(np.int64(0)...64(720)), ...} == {(np.int64(0)...64(720)), ...}
FAILED test_distances_issue4906.py::TestCappedDistances::test_capped_distance_skewed_triclinic[pkdtree-box2] - AssertionError: assert {(np.int64(0)...64(733)), ...} == {(np.int64(0)...64(733)), ...}
FAILED test_distances_issue4906.py::TestCappedDistances::test_capped_distance_skewed_triclinic[nsgrid-box0] - AssertionError: assert {(np.int64(0)...64(837)), ...} == {(np.int64(0)...64(837)), ...}
FAILED test_distances_issue4906.py::TestCappedDistances::test_capped_distance_skewed_triclinic[nsgrid-box1] - AssertionError: assert {(np.int64(0)...64(720)), ...} == {(np.int64(0)...64(720)), ...}
FAILED test_distances_issue4906.py::TestCappedDistances::test_capped_distance_skewed_triclinic[nsgrid-box2] - AssertionError: assert {(np.int64(0)...64(733)), ...} == {(np.int64(0)...64(733)), ...}
FAILED test_distances_issue4906.py::TestCappedDistances::test_self_capped_distance_skewed_triclinic[nsgrid-box0] - AssertionError: assert {(np.int64(0)...64(204)), ...} == {(np.int64(0)...64(204)), ...}
FAILED test_distances_issue4906.py::TestCappedDistances::test_self_capped_distance_skewed_triclinic[nsgrid-box1] - AssertionError: assert {(np.int64(0)...64(204)), ...} == {(np.int64(0)...64(204)), ...}
FAILED test_distances_issue4906.py::TestCappedDistances::test_self_capped_distance_skewed_triclinic[nsgrid-box2] - AssertionError: assert {(np.int64(0)...64(449)), ...} == {(np.int64(0)...64(449)), ...}
FAILED test_distances_issue4906.py::Test_apply_PBC::test_coords_in_central_image_skewed_tric[box0-serial] - assert np.False_
FAILED test_distances_issue4906.py::Test_apply_PBC::test_coords_in_central_image_skewed_tric[box0-openmp] - assert np.False_
FAILED test_distances_issue4906.py::Test_apply_PBC::test_coords_in_central_image_skewed_tric[box1-serial] - assert np.False_
FAILED test_distances_issue4906.py::Test_apply_PBC::test_coords_in_central_image_skewed_tric[box1-openmp] - assert np.False_
FAILED test_distances_issue4906.py::Test_apply_PBC::test_coords_in_central_image_skewed_tric[box2-serial] - assert np.False_
FAILED test_distances_issue4906.py::Test_apply_PBC::test_coords_in_central_image_skewed_tric[box2-openmp] - assert np.False_
=================================================== 17 failed, 5359 passed, 1 skipped, 1248 warnings in 14.34s ===================================================

BradyAJohnston · 2026-10-19T09:38:32Z

Thanks for the thorough review and for updating the docs @orbeckt . I've just updated that test to definitely fail on develop and now pass. The bug was only showing up for certain cutoff values so I had managed to pick a combo which just slipped under on develop initially. Should have double checked it failed first though.

Should be all good to go.

…nston/mdanalysis into fix-4906-triclinic-pbc

orbeckst · 2026-10-19T09:58:57Z

You beat me to it, I was just about to push a similarly updated test.

I have one that is guaranteed to fail with specially engineered points so I'll add it.

BradyAJohnston · 2026-10-19T10:09:23Z

Always good to have more tests!

orbeckst · 2026-10-19T17:46:42Z

🎉

@IAlibay I hope this is still acceptable.

fix triclinic pkdtree distances

e58a4f0

BradyAJohnston requested review from IAlibay and orbeckst October 9, 2026 11:07

Merge branch 'develop' into fix-4906-triclinic-pbc

e1ad003

BradyAJohnston changed the title ~~Fix: #4096: Incorrect capped_distance in some triclinic box situations~~ Fix: Incorrect capped_distance in some triclinic box situations Oct 9, 2026

update incorrect expectations

a3e37a8

IAlibay requested review from tylerjereddy and removed request for IAlibay October 9, 2026 11:42

BradyAJohnston added this to the 2.11 milestone Oct 13, 2026

fix box widths slowdown; cleanup tests and add versionchanged to docs

4940789

Merge branch 'fix-4906-triclinic-pbc' of https://github.com/BradyAJoh…

e3b07f8

…nston/mdanalysis into fix-4906-triclinic-pbc

orbeckst assigned orbeckst and BradyAJohnston Oct 19, 2026

orbeckst added 3 commits October 18, 2026 19:41

Merge branch 'develop' into fix-4906-triclinic-pbc

d0bd01c

Merge branch 'fix-4906-triclinic-pbc' of https://github.com/BradyAJoh…

76b86ff

…nston/mdanalysis into fix-4906-triclinic-pbc

additional docs for PBC fix

a7b88e1

- extensive CHANGELOG entry - versionchanged (because the fix is important) - code docs/comment updates

orbeckst force-pushed the fix-4906-triclinic-pbc branch from 8a50ba1 to a7b88e1 Compare October 19, 2026 08:29

add rhombic dodecahedron to tests (and made flake8 happy)

955d89c

orbeckst approved these changes Oct 19, 2026

View reviewed changes

update test to properly fail on develop

3ad297e

orbeckst added 2 commits October 19, 2026 02:56

updated tests for too thin cells

5635e45

Merge branch 'fix-4906-triclinic-pbc' of https://github.com/BradyAJoh…

502ea93

…nston/mdanalysis into fix-4906-triclinic-pbc

Merge branch 'develop' into fix-4906-triclinic-pbc

dfb5613

BradyAJohnston merged commit 6cd1f7e into MDAnalysis:develop Oct 19, 2026
25 checks passed

BradyAJohnston deleted the fix-4906-triclinic-pbc branch October 19, 2026 11:04

Conversation

BradyAJohnston commented Oct 9, 2026

LLM / AI generated code disclosure

PR Checklist

Developers Certificate of Origin

Uh oh!

read-the-docs-community Bot commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Documentation build overview

Uh oh!

BradyAJohnston commented Oct 9, 2026

Uh oh!

codecov Bot commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

IAlibay commented Oct 9, 2026

Uh oh!

orbeckst commented Oct 12, 2026

Uh oh!

orbeckst commented Oct 12, 2026

Uh oh!

IAlibay commented Oct 13, 2026

Uh oh!

orbeckst commented Oct 14, 2026

Uh oh!

IAlibay commented Oct 14, 2026

Uh oh!

orbeckst commented Oct 14, 2026

Uh oh!

BradyAJohnston commented Oct 15, 2026

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

orbeckst Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

orbeckst Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

orbeckst Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

orbeckst Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

orbeckst Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

orbeckst Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

orbeckst commented Oct 19, 2026

Uh oh!

BradyAJohnston commented Oct 19, 2026

Uh oh!

orbeckst commented Oct 19, 2026

Uh oh!

BradyAJohnston commented Oct 19, 2026

Uh oh!

Uh oh!

orbeckst commented Oct 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

read-the-docs-community Bot commented Oct 9, 2026 •

edited

Loading

codecov Bot commented Oct 9, 2026 •

edited

Loading