-
Notifications
You must be signed in to change notification settings - Fork 649
fix(pair-tab): validate uniform distance grid to avoid silently wrong potentials #5908
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
hanaol
wants to merge
12
commits into
deepmodeling:master
Choose a base branch
from
hanaol:fix/pairtab-nonuniform-grid
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
492e2e5
fix(pair-tab): validate uniform distance grid to avoid silently wrong…
hanaol f87eeb7
fix(pair-tab): reject non-monotonic grids and keep reinit atomic
hanaol a4e2644
Merge branch 'master' into fix/pairtab-nonuniform-grid
hanaol 3660ba2
test(pair-tab): snapshot serialized state before failed reinit
hanaol 772e1c2
style(pair-tab): drop explanatory comments from validation and tests
hanaol 040569e
fix(pair-tab): use a scale-aware tolerance for the grid spacing check
hanaol 925966d
Merge branch 'master' into fix/pairtab-nonuniform-grid
hanaol b00aeb3
Merge branch 'master' into fix/pairtab-nonuniform-grid
hanaol 2aa6704
fix(pair-tab): validate grid by absolute node position, not interval
hanaol 3527afe
fix(pair-tab): add explicit dtype to np.arange in grid validation
hanaol 915ebbe
fix(pair-tab): store the validated grid step instead of the first int…
hanaol be4d7bf
docs(pair-tab): document the uniform distance grid requirement
hanaol File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the line that keeps the fix from closing the bug it targets, and the reason it is written this way is my comment last round, which I am withdrawing.
The check above proves every node sits within one percent of a cell of
rmin + i * hh_ref. But what gets stored, and what every consumer then uses, is the first interval:tab_info[1]feedsuu = (rr - rmin) / hhinpair_tab.ccand the dpmodel/pt_pair_tabulated_inter,_make_datascales derivatives by it,rcut_idxand the paddinglinspacein_check_table_upper_boundaryare computed from it. Nothing bounds how far this value may sit fromhh_ref; the check only bounds node positions, so the first interval can be off by nearly a full percent while the table passes. A relative error inhhaccumulates linearly in the index.I reproduced it against this head:
linspace(0, 1, 1001)with only row 1 moved by 9.9e-6, i.e. 0.99 of a cell, is accepted,self.hhcomes out as 0.0010099, and atr = 0.995the consumer's index arithmetic lands on cell 985 while the node is 995. Ten cells off, silently, which is precisely the failure mode in the PR title. The neighbouring comment says the consumers'rmin + i * hhis "what must stay accurate", and it is, but it ishh_refthat was made accurate, nothh.The fix is one line:
self.hh = hh_ref, and drop the first-intervalhh. I asked last round to keep them separate to avoid changingtab_infofor existing models; having worked through it, that caution was misplaced. For a table with a round stride the two values are bit-identical, and for a printed tablehh_refaverages out the rounding of row 1 and is the better estimate, so the change to existing models is at the printing-precision level and in the right direction. The error message already reportshh_refas the step, so this also makes what the object stores agree with what it tells the user.A test that would have caught it: a uniform grid with a single node perturbed inside the tolerance, asserting
tab.hhagainst the true step withrtolrather thanassertAlmostEqual.