fix(infer): normalize parameter shorthand before batching - #5857
fix(infer): normalize parameter shorthand before batching#5857njzjz-bot wants to merge 8 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughDeepEval gains shared validation and expansion for ChangesParameter shorthand normalization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant DeepEval
participant Standardizer
participant Backend
Caller->>DeepEval: provide fparam and aparam
DeepEval->>Standardizer: pass frame, atom, and model dimensions
Standardizer-->>DeepEval: return standardized parameters
DeepEval->>Backend: evaluate normalized inputs
Backend-->>Caller: return evaluation outputs
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 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 `@deepmd/pt/infer/deep_eval.py`:
- Around line 548-555: Update eval_embedding to call _standardize_fparam_aparam
on fparam and aparam before dispatching to self._eval_func, using the embedding
evaluation’s frame count, natoms, and model parameter dimensions. Pass the
standardized values to the backend so shorthand inputs are batched by frames
correctly.
🪄 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
Run ID: e2d24567-276c-4ec1-a33e-25ee33258076
📒 Files selected for processing (7)
deepmd/infer/deep_eval.pydeepmd/jax/infer/deep_eval.pydeepmd/pd/infer/deep_eval.pydeepmd/pt/infer/deep_eval.pydeepmd/tf2/infer/deep_eval.pysource/tests/common/test_deep_eval_parameter_shorthand.pysource/tests/consistent/io/test_io.py
Standardize frame and atomic parameter shorthand in the common wrapper and backend entry points before automatic batching. Preserve full per-frame and per-atom arrays while broadcasting documented shared forms consistently across TensorFlow 2, PyTorch, JAX, and Paddle. Normalize PyTorch embedding extraction before split execution so eval_embedding, eval_descriptor, and eval_fitting_last_layer accept shared fparam, per-atom aparam, and scalar aparam forms. Cover two frames forced through one-frame backend batches. Coding-Agent: Codex Codex-Version: codex-cli 0.144.4 Model: gpt-5.6-sol Reasoning-Effort: xhigh
2149d24 to
9227907
Compare
for more information, see https://pre-commit.ci
|
Possible reviewers based on changed lines, exact file history, and exact-file review history:
No review request was made automatically. Coding agent: Codex |
…shorthand-backends-5666
deserialize_to_file only writes .json, serialize_from_file raises NotImplementedError, and the .json reader rejects fparam/aparam, so the paddle entry could not complete the round trip. Its normalization stays covered by test_deep_eval_parameter_shorthand.py.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/tests/consistent/io/test_io.py (1)
264-267: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse non-uniform parameter values in this regression test.
Because all parameters are filled with ones, incorrect frame/atom/feature tiling or axis permutation can still produce identical outputs. Use distinct values so the test validates normalization semantics, not just shapes.
Proposed test data fix
- fparam_shared = np.ones(deep_eval.get_dim_fparam()) - aparam_per_atom = np.ones((natoms, deep_eval.get_dim_aparam())) + fparam_shared = np.arange( + 1, deep_eval.get_dim_fparam() + 1, dtype=GLOBAL_NP_FLOAT_PRECISION + ) + aparam_per_atom = np.arange( + 1, + natoms * deep_eval.get_dim_aparam() + 1, + dtype=GLOBAL_NP_FLOAT_PRECISION, + ).reshape(natoms, deep_eval.get_dim_aparam()) ... - np.ones(deep_eval.get_dim_aparam()), + np.arange( + 1, + deep_eval.get_dim_aparam() + 1, + dtype=GLOBAL_NP_FLOAT_PRECISION, + ),Also applies to: 278-282
🤖 Prompt for 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. In `@source/tests/consistent/io/test_io.py` around lines 264 - 267, Update the regression test data around fparam_shared, aparam_per_atom, fparam_full, and aparam_full to use distinct, non-uniform values across frames, atoms, and features; apply the same change to the corresponding setup at the additional referenced location so incorrect tiling or axis permutation produces different outputs and the test validates normalization semantics.
🤖 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.
Outside diff comments:
In `@source/tests/consistent/io/test_io.py`:
- Around line 264-267: Update the regression test data around fparam_shared,
aparam_per_atom, fparam_full, and aparam_full to use distinct, non-uniform
values across frames, atoms, and features; apply the same change to the
corresponding setup at the additional referenced location so incorrect tiling or
axis permutation produces different outputs and the test validates normalization
semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8490894b-d101-4255-a490-f334eda1c482
📒 Files selected for processing (2)
deepmd/infer/deep_eval.pysource/tests/consistent/io/test_io.py
🚧 Files skipped from review as they are similar to previous changes (1)
- deepmd/infer/deep_eval.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5857 +/- ##
==========================================
+ Coverage 79.03% 79.24% +0.20%
==========================================
Files 1055 1072 +17
Lines 122233 125055 +2822
Branches 4401 4541 +140
==========================================
+ Hits 96607 99098 +2491
- Misses 24061 24332 +271
- Partials 1565 1625 +60 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
The core change is right, and putting the ladder in one shared helper called before AutoBatchSize is the correct design. I checked it rather than assuming: full-shape inputs are unchanged across a wide sweep of dim_fparam / dim_aparam / nframes / auto_batch_size combinations, the public path stays idempotent (the flattened 2-D form re-enters on the nframes*natoms*dim rung and only reshapes, no double-tiling), the three size-dispatch collision families (nframes==1, natoms==1, both) produce identical arrays in every colliding branch, and the spin route gets the real-atom natoms that _eval_model_spin expects. It also fixes the long-standing "got wrong size of frame param" typo in the aparam branch.
I ran the new test_embedding.py case against the pre-fix tree and it fails with ValueError: cannot reshape array of size 1 into shape (1,7,1), so that one is a genuine regression test - two frames, a forced per-frame split, and np.linspace(0.1, 0.7, natoms) so aparam actually varies per atom. Good test.
My comments are all about coverage and scope, not the helper. Six inline. The two I would weigh most are the ones about what the tests can actually detect: test_io.py uses uniform values, and on a GPU runner neither new test performs the split it is built around.
Note the head moved while I was reviewing (bbfc000ac2b1 -> 8d80b8579); everything below is against 8d80b8579.
Smaller notes, not worth their own threads:
-
The TF v1 backend has the same defect and is worse than the others:
deepmd/tf/infer/deep_eval.pypassesfparam=/aparam=as kwargs intoexecute_all(which slices kwargs too), and its shorthand ladder lives inside_prepare_feed_dict, i.e. per batch, after slicing. With one frame per batch a(natoms, dim_aparam)array is sliced on its atom axis to(1, dim_aparam), which then matches thesize == dim_aparamrung and tiles atom 0's value onto every atom - wrong numbers, no exception. Out of this PR's stated scope, but"tensorflow"is in the same test loop and its exclusion is the undocumented one. -
The PR body says it covers "both normal and spin backend routes". The placement before the spin branch is correct, but no test in the diff constructs a spin model or passes
spin=, so_eval_model_spinis never exercised with shorthand. -
test_embedding.py'sassertEqual(backend.auto_batch_size.current_batch_size, natoms)fails outright ifDP_INFER_BATCH_SIZEis exported, sinceAutoBatchSize.__init__lets that variable override the constructor argument - and if it is set, the test silently stops forcing the split while still passing. -
The helper unit tests never pass
fparamandaparamtogether, which is the shape every production call site uses. The two branches are independent so nothing is likely hiding there. -
_standard_inputbuilds the canonical 3-D aparam and immediately re-flattens it to 2-D. Every consumer reshapes from.sizeregardless of incoming rank, so nothing needs the 2-D form any more; dropping the re-flatten would make the internal boundary unambiguous. -
The aparam error text changes from "frame param" to "atomic param" (a real fix), but
deepmd/tf/infer/deep_eval.pystill carries the old wording in its own duplicate check, so the two entry points now disagree. Worth a line in the PR description since it is user-visible.
| boxes = np.repeat(self.box, nframes, axis=0) | ||
| atom_types = self.atype.reshape(-1) | ||
| fparam_shared = np.ones(deep_eval.get_dim_fparam()) | ||
| aparam_per_atom = np.ones((natoms, deep_eval.get_dim_aparam())) |
There was a problem hiding this comment.
Every value in this test is 1.0, and coords/boxes above are np.repeat of a single frame, so the frames are byte-identical too. That means the comparison can only catch a crash or a shape error - an implementation that tiled along the wrong axis, permuted frame and atom axes, or reused frame 0's parameters for every frame would still produce identical outputs and pass.
That matters because frame-major and atom-major correctness under batching is the whole point of the change.
The sibling test in source/tests/pt/model/test_embedding.py already does this right - fparam = [0.25, -0.5] and aparam = np.linspace(0.1, 0.7, natoms) - so the fix is just to use the same idea here: distinct values per atom for aparam_per_atom, and distinct rows per frame for the full reference, so a mis-tiling shows up as a wrong number rather than a coincidence.
I raised the same point on #5853 and CodeRabbit flagged this exact block on this PR; it is still unaddressed at head, so flagging once more.
There was a problem hiding this comment.
Fixed in 9867233. The consistency regression now uses distinct coordinates, boxes, per-atom values, and distinct full frame-major parameter rows. It checks shared-per-atom, shared-all-atoms, and full-frame-major cases, so axis swaps and frame reuse change the numerical result.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
| self.assertIsInstance(backend, PTDeepEval) | ||
| self.assertEqual(backend.auto_batch_size.current_batch_size, natoms) | ||
|
|
||
| full = backend.eval_embedding( |
There was a problem hiding this comment.
On a GPU runner this test stops performing the split its comment describes.
AutoBatchSize.execute grows current_batch_size by factor after every successful batch while current_batch_size * factor < minimal_not_working_batch_size, and that bound is 2**31 when is_gpu_available(). Measured against the real batch_size.py with a stubbed GPU flag, 2 frames x 6 atoms, initial_batch_size=6: 6 -> 12 -> 24.
Because the full-shape reference call is issued first on this same backend instance, by the time the shorthand calls run the batch size has already grown past nframes, so they execute unbatched and the "batcher mistakes the atom axis for frames" scenario never happens. .github/workflows/test_cuda.yml runs source/tests on a GPU runner and this test is not skipped there, so this is the actual behaviour in CI rather than a hypothetical.
It still catches the plain reshape failure - it does fail pre-fix, I checked - but the batching half of its rationale is lost. Either issue the shorthand call first, or rebuild the DeepEval inside each case so the batch size starts fresh.
The same ordering applies to _assert_backend_parameter_shorthand in source/tests/consistent/io/test_io.py. One knock-on there: once reference and shorthand runs use different batch shapes, that test compares them at rtol=1e-12, atol=1e-12, and bitwise agreement across differing GEMM shapes is not guaranteed for every backend and dtype.
There was a problem hiding this comment.
Fixed in 9867233. Every shorthand/full input is now evaluated first on a fresh auto-batcher, with a separate unbatched evaluator for the reference, so GPU batch-size growth cannot remove the intended split. The cross-batch comparison tolerance is relaxed to 1e-10 for backend-dependent GEMM grouping.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
| # writes .json, serialize_from_file is not implemented, and the | ||
| # .json reader rejects fparam/aparam. Its normalization is covered | ||
| # by source/tests/common/test_deep_eval_parameter_shorthand.py. | ||
| if backend_name in {"pytorch", "jax", "tf2"} and ( |
There was a problem hiding this comment.
"dpmodel" is in the backend loop just above but is excluded here, and unlike paddle that exclusion carries no comment. The reason is that dpmodel was not fixed: deepmd/dpmodel/infer/deep_eval.py still passes raw fparam/aparam into _eval_func / AutoBatchSize and then does a bare reshape(nframes, natoms, dim) - the same defect this PR removes from the other four adapters. "tensorflow" is in the loop and excluded for the same unstated reason.
The consequence is worth deciding deliberately, because #5853 fixes exactly that file with its own inline copy of this ladder. The two PRs touch disjoint files and will merge cleanly, but the result is a shared _standardize_fparam_aparam plus a hand-inlined duplicate of it in dpmodel - the duplication this PR exists to remove.
Cleanest resolutions are to wire dpmodel into the helper here and close #5853, or rebase #5853 onto the helper. Either way, please make the allowlist say why each backend is out, so the gap is a recorded decision rather than an invisible one.
There was a problem hiding this comment.
Fixed in 9867233. dpmodel now uses the shared helper before auto batching, and the consistency allowlist now exercises every backend present in the loop. I also wired the legacy TensorFlow adapter because it had the same normalization-after-batching ordering.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
| aparam = None | ||
| # Paddle is absent from the loop above: deserialize_to_file only | ||
| # writes .json, serialize_from_file is not implemented, and the | ||
| # .json reader rejects fparam/aparam. Its normalization is covered |
There was a problem hiding this comment.
This comment claims more than the referenced file delivers. source/tests/common/test_deep_eval_parameter_shorthand.py imports _standardize_fparam_aparam from deepmd/infer/deep_eval.py and calls it directly - it never imports or executes a single line of deepmd/pd/infer/deep_eval.py. So it shows the helper is correct, but not that the pd adapter calls it, which is the part this PR added.
After 8d80b8579 dropped the paddle round trip, the pd hunk has no test that executes it at all. The PR body also still says "Paddle is now included in the same consistency path when its dependency is available", which is no longer true.
Either restore some executing coverage for pd, or reword both the comment and the PR body to say plainly that the pd change is untested and why.
There was a problem hiding this comment.
Fixed in 9867233. Added a dedicated Paddle adapter test that constructs the backend, calls its eval method, and verifies the normalized frame-major arrays passed to _eval_model. I also corrected the PR body: Paddle is not installed locally, so this test will execute in the Paddle test environment rather than the local consistency loop.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
| import ase.neighborlist | ||
|
|
||
|
|
||
| def _standardize_fparam_aparam( |
There was a problem hiding this comment.
deepmd/pt_expt/infer/deep_eval.py has the same defect and is not wired to this helper: raw fparam/aparam go into _eval_func / AutoBatchSize, and _eval_model / _eval_model_lower do bare fparam.reshape(nframes, dim_fparam) and aparam.reshape(nframes, natoms, dim_aparam) with no shorthand handling. It is also absent from the io test loop, so nothing would catch it.
Its own eval docstring documents only the full nframes x ... forms, which quietly narrows the base DeepEvalBackend.eval contract - that has listed all three shorthand forms since #3213 and is the contract this PR is enforcing everywhere else.
Given pt_expt is the actively developed backend and carries no back-compat constraints, adding the same one-line call here seems worth doing in this PR rather than leaving a fifth adapter behind.
There was a problem hiding this comment.
Fixed in 9867233. pt_expt now calls the shared helper before auto batching, its eval docstring documents all supported shorthand forms, and a direct adapter regression verifies the normalized arguments reaching _eval_model.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
| dim_fparam: int, | ||
| dim_aparam: int, | ||
| ) -> tuple[np.ndarray | None, np.ndarray | None]: | ||
| """Normalize documented parameter shorthand to frame-major arrays. |
There was a problem hiding this comment.
The docstring explains the rationale well but skips the numpydoc Parameters/Returns sections that the neighbouring functions in this file use - eval, eval_descriptor, eval_fitting_last_layer, eval_embedding, and even the private single-argument _check_mixed_types.
With six parameters and a two-element tuple return, and given this is now the single definition of the shorthand contract for four backends, it is worth documenting each argument and the returned shapes explicitly - in particular that aparam comes back 3-D (nframes, natoms, dim_aparam) while _standard_input re-flattens it to 2-D for the public path.
There was a problem hiding this comment.
Fixed in 9867233. The shared helper now has complete numpydoc Parameters and Returns sections, including the canonical 2-D fparam and 3-D aparam return shapes and the public wrapper flattening note.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
Normalize parameter shorthand in the remaining dpmodel, pt_expt, and TensorFlow adapters before automatic batching. Strengthen the regressions with distinct frames and parameters, fresh batchers, and direct Paddle and pt_expt adapter coverage. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
Stack the dpmodel-specific regression on the shared backend normalization from deepmodeling#5857. Use distinct coordinates and frame-major parameters, run shorthand inputs on fresh batchers, and verify split evaluation preserves frame order and distinct energies. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
Coding-Agent: Codex\nCodex-Version: codex-cli 0.144.6\nModel: gpt-5.6-sol\nReasoning-Effort: xhigh
Stack the dpmodel-specific regression on the shared backend normalization from deepmodeling#5857. Use distinct coordinates and frame-major parameters, run shorthand inputs on fresh batchers, and verify split evaluation preserves frame order and distinct energies. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Thanks -- I went through all six of my earlier threads against HEAD f785c9e0b rather than the replies, and they are genuinely addressed:
- The consistency test no longer compares byte-identical frames. Distinct coordinates (
+0.125), a perturbed box,linspaceparameters and distinct per-frame rows mean a mis-tiling now changes a number instead of coinciding. - The batching half of the rationale is restored: each case builds a fresh
DeepEval(model_file, auto_batch_size=natoms)and issues the shorthand call first, with a separate unbatched evaluator for the reference, so GPU batch-size growth cannot silently remove the split. Relaxing to1e-10for the cross-batch comparison is the right call given differing GEMM shapes. - The allowlist gap is closed the better way -- dpmodel and tensorflow are exercised rather than excluded, and Paddle's absence now carries a stated reason instead of being invisible.
- The pd hunk has executing coverage again, and the PR body is corrected to say plainly that it runs in the Paddle environment rather than locally.
_standard_inputitself now delegates to the helper and re-flattens to the historical 2-D ABI, so the duplication I was worried about is genuinely gone: one definition serving the public wrapper and every adapter.- The helper's numpydoc block covers the parameters and both return shapes, including the 3-D aparam and the flattening note.
I also checked the two things that would worry me about a refactor of this shape. An exhaustive sweep of the new helper against master's _standard_input ladder over nframes x natoms x dim_fparam x dim_aparam x input size x candidate shapes produced no difference in value, shape, or raise behaviour -- the .size dispatch is ambiguous only in the cases where both branches yield the same array, exactly as before. And on the regression side I ran your new pt_expt adapter test against unfixed code: it fails there, so it does prove the fix.
One gap left, inline, on the adapter I asked about last time. I would rather see it closed here than tracked separately, since it is the same contract and the same file.
Two smaller notes, neither needing action in this PR. deepmd/tf/infer/deep_eval.py#L850-L862 still holds the old private ladder; both of its callers now receive normalized input so it is unreachable in practice, but it still carries the copy-paste bug your helper fixes -- the aparam branch raises "got wrong size of frame param". Worth deleting or reducing to an assertion while the context is fresh. And PretrainedDeepEvalBackend (deepmd/pretrained/deep_eval.py#L55-L60) is an eighth DeepEvalBackend subclass not named in the PR body's "seven adapters"; it is a pure delegator so it inherits the fix, but it is worth a mention so nobody assumes it was audited and missed.
Unrelated to the diff: the Read the Docs check is failing on this head. I could not read the build log (the page is not publicly accessible), and #5908 fails it too while other open PRs pass, so it may well be infrastructure rather than yours -- flagging only so it is not overlooked.
| natoms, numb_test = self._get_natoms_and_nframes( | ||
| coords, atom_types, len(atom_types.shape) > 1 | ||
| ) | ||
| fparam, aparam = _standardize_fparam_aparam( |
There was a problem hiding this comment.
This wires eval, but not the other two entry points on this backend, and the PR body says otherwise.
The summary claims the change covers "normal, spin, embedding, descriptor, and fitting-last-layer routes". For pt_expt the descriptor and fitting-last-layer routes are not covered: _standardize_fparam_aparam appears exactly twice in this file -- the import and this call. Meanwhile eval_descriptor and eval_fitting_last_layer both hand their raw fparam/aparam to _prepare_nlist_inputs, which does the hard reshape at L1674-L1678:
if aparam is not None:
aparam_t = torch.tensor(
aparam.reshape(nframes, natoms, self.get_dim_aparam()),So a backend-direct eval_descriptor(coords_2frames, cells, atypes, aparam=np.zeros((natoms, dim_aparam))) raises ValueError: cannot reshape array of size ... into shape (2, natoms, dim), while the base-class method it overrides documents that exact form (deepmd/infer/deep_eval.py). pt handles it after this PR and pd inherits it by delegating to self.eval, so pt_expt is now the only backend that rejects documented input on these two routes.
I want to be clear this is milder than the bug the PR fixes: these routes do not go through AutoBatchSize, so the failure is a loud rejection rather than silent corruption, and the public eval_descriptor path is unaffected because _standard_input pre-flattens. It is a contract inconsistency, not a data-corruption bug. But it is the same one-line call in the same file, and leaving it means the PR body overstates what landed -- which is the part I would most like fixed either way.
Normalizing inside _prepare_nlist_inputs would cover both entry points at once and match how pd gets it for free. Their docstrings would want the shorthand lines too -- they currently say only "Frame parameters, optional.", the same narrowing you just corrected for eval.
On coverage while you are here: the new pd and pt_expt adapter tests both set _eval_func = lambda inner, numb_test, numb_atoms: inner, which takes AutoBatchSize out of the picture entirely. They prove the helper is called, which is useful, but they cannot catch a regression in the axis-slicing behaviour in execute_all that made this a bug in the first place -- only test_io.py and test_embedding.py do that, via a real auto_batch_size=natoms. Pointing one of the two adapter tests at a real batcher would close that.
There was a problem hiding this comment.
Fixed in 866ce34. _standardize_fparam_aparam is now wired into pt_expt's eval_descriptor and eval_fitting_last_layer (in addition to eval), so both routes normalize shared-per-atom shorthand before _prepare_nlist_inputs reshapes the arrays. Added adapter regressions for both routes that call the backend methods directly and assert the normalized frame-major arrays reach _prepare_nlist_inputs; all three pt_expt adapter tests pass and ruff is clean. The PR body's "descriptor and fitting-last-layer routes" claim now matches the implementation.
Coding agent: opencode
opencode version: 1.18.9
Model: ustc/deepseek-v4-flash
Reasoning effort: max
…r routes Wire _standardize_fparam_aparam into eval_descriptor and eval_fitting_last_layer so shared per-atom shorthand cannot be mistaken for a frame axis before _prepare_nlist_inputs reshapes it. Extend the pt_expt adapter regressions to cover both routes. Coding-Agent: opencode opencode-Version: 1.18.9 Model: ustc/deepseek-v4-flash Reasoning-Effort: max
The backend-direct shorthand comparison never checked that the two input frames actually differ, so a degenerate expansion that reuses one frame's parameters for every frame would also make the frame-major reference degenerate and slip past the allclose comparison. Assert the per-frame energy values differ on every shorthand case. Coding-Agent: opencode opencode-Version: 1.18.9 Model: ustc/deepseek-v4-flash Reasoning-Effort: max
Stack the dpmodel-specific regression on the shared backend normalization from deepmodeling#5857. Use distinct coordinates and frame-major parameters, run shorthand inputs on fresh batchers, and verify split evaluation preserves frame order and distinct energies. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
3 participants
Closes #5666.
Summary
Why existing tests missed this
Existing multi-frame inference tests call the high-level DeepPot.eval wrapper, whose _standard_input already expands shorthand before backend dispatch. Backend-direct consistency tests used canonical full shapes, while many backend-specific fixtures use only one frame, where shorthand sizes can coincide with full sizes.
The regressions deliberately call backend methods directly, use distinct coordinates and parameter values, and invoke shorthand/full inputs first on fresh auto_batch_size=natoms evaluators. This catches axis swaps, frame reuse, and GPU auto-batch growth that could otherwise make the intended split disappear.
Validation
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
Summary by CodeRabbit
New Features
Bug Fixes
Tests