fix(pair-tab): validate uniform distance grid to avoid silently wrong potentials - #5908
fix(pair-tab): validate uniform distance grid to avoid silently wrong potentials#5908hanaol wants to merge 8 commits into
Conversation
… potentials Signed-off-by: hanaol <ho0950@princeton.edu>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesPair table grid validation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deepmd/utils/pair_tab.py`:
- Around line 64-72: Update the radial-grid validation in the pair-table
initialization around vdata and dx to explicitly reject any non-positive
distance interval before or alongside the existing uniform-spacing check.
Preserve the current ValueError behavior for uneven grids, ensure duplicate and
descending grids cannot proceed to hh-dependent division or padding, and add
regression coverage for both cases.
- Around line 64-72: Make reinit() validate the newly loaded table using local
variables before modifying self.vdata, self.rmin, self.rmax, or self.hh. Move
the non-uniform-grid check and any related validation ahead of committing
instance state, then update all instance metadata and tab_info/tab_data together
only after validation succeeds, preserving the existing object unchanged when
validation raises.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 259347aa-00a8-4fe1-b26d-3bf7c9ddeb92
📒 Files selected for processing (2)
deepmd/utils/pair_tab.pysource/tests/common/dpmodel/test_pairtab_preprocess.py
Address review feedback on the pairwise table validation: - A constant zero or negative distance stride passed the uniform-spacing check, leaving hh == 0 (division by zero) or hh < 0 with rmin > rmax in the padding and extrapolation arithmetic. Require a strictly increasing grid before checking uniformity. - reinit() assigned vdata/rmin/rmax/hh before validating, so a failed reinit of a live PairTab left the new metadata next to the stale tab_info/tab_data. Validate locals first and commit instance state only once all checks pass. Add regression tests for duplicate and descending grids, and assert that a failed reinit leaves the serialized table unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: hanaol <ho0950@princeton.edu>
|
Thanks for the review — both findings addressed in f87eeb7, with two intentional deviations from the suggested diffs:
Regression tests added for the duplicate-distance and descending-grid cases, plus one asserting that a failed |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/tests/common/dpmodel/test_pairtab_preprocess.py`:
- Around line 334-354: Update the test’s serialized-state snapshot by importing
copy and deep-copying the result of PairTab.serialize() before reinit(), so
expected arrays cannot alias live state. Replace np.testing.assert_allclose
comparisons for vdata, tab_info, and tab_data with exact array comparisons while
preserving the existing scalar checks; run the focused pytest case, ruff check
., and ruff format .
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2cf3e420-5b67-4128-a098-f7b7e161b1da
📒 Files selected for processing (2)
deepmd/utils/pair_tab.pysource/tests/common/dpmodel/test_pairtab_preprocess.py
🚧 Files skipped from review as they are similar to previous changes (1)
- deepmd/utils/pair_tab.py
serialize() returns references to the live vdata/tab_info/tab_data arrays, so deep-copy the expected snapshot rather than aliasing it, and compare the arrays exactly since a failed reinit must leave them untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: hanaol <ho0950@princeton.edu>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: hanaol <ho0950@princeton.edu>
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
Request changes
The uniform-grid check has a scale-dependent false-negative: atol=1e-8 permits clearly non-uniform grids whose spacing is below that absolute tolerance. Please make the tolerance relative to hh (for example, use atol=0 while retaining the relative tolerance) and add a regression test with a small non-uniform step.
Reviewed by OpenClaw 2026.6.11.
atol=1e-8 dominated the comparison for sub-nanometre grids, so intervals differing by an order of magnitude still compared equal and the table was encoded with the smaller stride. Drop the absolute term and rely on rtol, which is scale-invariant. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: hanaol <ho0950@princeton.edu>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5908 +/- ##
==========================================
- Coverage 79.21% 78.96% -0.25%
==========================================
Files 1069 1069
Lines 124070 124080 +10
Branches 4522 4527 +5
==========================================
- Hits 98278 97980 -298
- Misses 24171 24483 +312
+ Partials 1621 1617 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Summary
PairTab(used foruse_srtaband ZBLtab_fileshort-range tables) assumes the distance grid is uniformly spaced, but never validates it. A non-uniform table is silently accepted and produces incorrect potentials with no error or warning.Problem
PairTab.reinit()infers a single stride from the first two rows only:self.hhis then baked into the spline coefficients and stored in tab_info, and the C++ inference kernel (source/lib/src/pair_tab.cc) maps distance to table index purely asidx = floor((r - rmin) / hh). No per-row distances are kept, so a non-uniform grid is impossible to represent.Also, the uniform-grid requirement is documented (doc/model/pairtab.md: the table is defined "on an evenly discretized grid"), but nothing enforced it.
Fix
Validate in
reinit()that every distance interval matches the inferred stride, and raise aValueErrorotherwise. The check runs before any spline is built, so a bad table is rejected up front instead of mis-indexed later.Tests
Added
TestPairTabGridSpacinginsource/tests/common/dpmodel/test_pairtab_preprocess.py:
test_non_uniform_grid — a table with a changing stride now raises ValueError.
test_uniform_grid — a uniformly spaced table is still accepted.
Summary by CodeRabbit
ValueErroradvising to regrid.