Fix PersistenceLength normalization for sliced run (Issue #5453) - #5459
Fix PersistenceLength normalization for sliced run (Issue #5453)#5459srikarjy wants to merge 3 commits into
Conversation
…5453) PersistenceLength._conclude() normalized bond_autocorrelation using the total number of frames in the trajectory instead of the number of frames actually analyzed. This gave wrong results whenever run() was called with start, stop, or step. Now uses self.n_frames, which AnalysisBase already tracks for this purpose. Added a regression test comparing full trajectory and sliced runs against manually computed normalization values.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5459 +/- ##
========================================
Coverage 93.87% 93.87%
========================================
Files 182 182
Lines 22522 22524 +2
Branches 3206 3207 +1
========================================
+ Hits 21143 21145 +2
Misses 917 917
Partials 462 462 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Documentation build overview
|
| * 0.7.5.1 | ||
|
|
||
| Fixes | ||
| * `PersistenceLength._conclude()` now normalizes `results.bond_autocorrelation` using the number of frames actually analyzed instead of the total trajectory frame count, fixing incorrect results when `run()` is called with `start`/`stop`/`step` (Issue #5453). |
There was a problem hiding this comment.
Why has this CHANGELOG entry been duplicated across so many old versions? Did you see any previous examples where we did this? Did you read your diff carefully?
There was a problem hiding this comment.
You are right, and I apologize. I did not review my diff carefully enough before submitting it, which is how the duplication slipped through. I removed the duplicate entries. The entry now appears only once, under the current unreleased 2.11.0 section, consistent with the rest of the file.
| norm *= len(chains) * n_sliced_frames | ||
| expected = p.results.raw_bond_autocorr / norm | ||
|
|
||
| assert_almost_equal(p.results.bond_autocorrelation, expected, decimal=6) |
There was a problem hiding this comment.
Here and above -- I don't like that your expected value is derived from the actual class/function under test. That can be a recipe for suppression of breaking changes if both the actual and expected values are mutated in a similar way.
There was a problem hiding this comment.
Agreed. Reworked this so expected is computed independently, directly from atom positions and trajectory frames, without calling into PersistenceLength or touching any of its results. Also extended the test to cover step=, start/stop/step, and frames=[...], since the original only exercised stop=.
* Remove the CHANGELOG entry that was accidentally duplicated across about 25 historical release sections. It now appears only once, under the current unreleased 2.11.0 header. * Replace the tautological regression test, where the expected value was derived from the class under test, with an independent reference implementation that recomputes the normalized bond autocorrelation directly from atom positions, without touching PersistenceLength or its results. * Extend test coverage to step=, start/stop/step, frames=[...], and the multiprocessing backend, beyond the original stop= only case. * Add a guard in _conclude() that raises a clear ValueError when a run selects zero frames, instead of silently returning a fabricated all zero curve before this fix, or crashing with an opaque scipy NaN error after this fix, found while testing the above. * Document, without fixing since it is out of scope for this issue, two existing unrelated edge cases found during testing: a one atom chain crashing identically before and after this fix, and a numpy boolean frames= mask being misread by AnalysisBase computation grouping, pinned as xfail for visibility.
|
Thanks again for the review. While reworking the test above, I also found and fixed something not covered by either of your original comments. An empty frame selection, for example run(start=2, stop=2), used to silently return a fabricated all zero curve. It now raises a clear ValueError instead of crashing later inside scipy.optimize.curve_fit with an opaque NaN error. I also extended the regression test to cover step=, start/stop/step, frames=[...], and the multiprocessing backend, since the original test only exercised stop=. While testing further I found two more things that already exist and are unrelated to this normalization bug, so I documented them with tests rather than fixing them here: a one atom chain crashes identically before and after this fix, and a numpy boolean frames= mask gets misread by AnalysisBase computation grouping logic. I pinned the second one as an xfail test so it stays visible. Happy to open separate issues for either if that is useful. Thanks again for catching both of the original issues. |
…ength-normalization-5453 # Conflicts: # package/CHANGELOG
2 participants
Fixes #5453
Changes made in this Pull Request:
LLM / AI generated code disclosure
LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: minimal (an AI assistant pointed me to the relevant line and explained the concept; I made the edits and wrote/ran the tests myself)
PR Checklist
package/CHANGELOGfile updated?package/AUTHORS?