Skip to content

fix(dpmodel): forward spin inputs in DeepEval#5852

Open
njzjz-bot wants to merge 2 commits into
deepmodeling:masterfrom
njzjz-bot:fix/dpmodel-deepeval-spin-inputs-5661
Open

fix(dpmodel): forward spin inputs in DeepEval#5852
njzjz-bot wants to merge 2 commits into
deepmodeling:masterfrom
njzjz-bot:fix/dpmodel-deepeval-spin-inputs-5661

Conversation

@njzjz-bot

Copy link
Copy Markdown
Contributor

Closes #5661.

Summary

  • normalize and validate spin inputs before automatic batching so flattened multi-frame inputs are sliced with their coordinate frames
  • forward extra model inputs through the dpmodel DeepEval adapter while keeping evaluator-owned inputs canonical
  • retain the magnetic-atom mask in non-atomic DeepPot results
  • add public-API regressions for forced one-frame batching and missing spin inputs

Why existing tests missed this

Existing dpmodel spin tests exercised direct model calls and serialization parity, not the DeepEval adapter. Public spin inference coverage targeted the PyTorch and PT2 backends, which already have dedicated spin forwarding paths. Those fixtures were also primarily single-frame, so they would not detect a flat spin tensor being left unsliced when automatic batching splits a multi-frame evaluation.

Validation

  • pytest source/tests/infer/test_dpmodel_deep_eval_spin.py -q (2 passed)
  • source/tests/common/test_auto_batch_size.py::TestAutoBatchSize::test_execute_all (passed)
  • dpmodel non-spin DeepEval smoke evaluation (passed)
  • ruff format . (1664 files unchanged)
  • ruff check . (passed)
  • git diff --check (passed)

Coding agent: Codex
Codex version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning effort: xhigh

Normalize spin tensors before automatic batching and pass supported extra
model inputs through the dpmodel evaluator. Preserve the magnetic mask in
non-atomic DeepPot results and add multi-frame regression coverage.

Coding-Agent: Codex
Codex-Version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning-Effort: xhigh
@dosubot dosubot Bot added the bug label Jul 16, 2026
@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 78.33%. Comparing base (6c3b985) to head (c09d167).

Files with missing lines Patch % Lines
deepmd/dpmodel/infer/deep_eval.py 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5852      +/-   ##
==========================================
- Coverage   78.58%   78.33%   -0.26%     
==========================================
  Files        1050     1050              
  Lines      120637   120648      +11     
  Branches     4356     4357       +1     
==========================================
- Hits        94801    94504     -297     
- Misses      24278    24583     +305     
- Partials     1558     1561       +3     

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@njzjz

njzjz commented Jul 18, 2026

Copy link
Copy Markdown
Member

Possible reviewers based on changed lines, exact file history, and exact-file review history:

  • @wanghan-iapcm — 5 commits on changed files; 26 reviews on exact changed files (deepmd/dpmodel/infer/deep_eval.py).
  • @iProzd — 3 commits on changed files (deepmd/dpmodel/infer/deep_eval.py).

No review request was made automatically.

Coding agent: Codex
Codex version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning effort: xhigh

@njzjz
njzjz requested review from iProzd and wanghan-iapcm and removed request for iProzd and wanghan-iapcm July 18, 2026 07:26
@wanghan-iapcm
wanghan-iapcm dismissed their stale review July 26, 2026 09:08

Retracted: submitted without maintainer sign-off.

@wanghan-iapcm wanghan-iapcm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diagnosis is right: the dpmodel DeepEval accepted **kwargs and then dropped them before calling the model, so SpinModel.call() - which takes spin as a required positional - was unreachable through the public evaluator. Normalizing spin to (nframes, natoms, 3) before AutoBatchSize is also the correct place for it, since execute_all only slices arrays with ndim > 1, so a flat spin array would otherwise be handed whole to every batch while coords were sliced frame by frame. Dispatching on self.get_has_spin() rather than on kwarg presence is better than what pt and pt_expt do, and the up-front ValueError is a genuine improvement over failing deep inside the model.

The two new tests do fail pre-fix - without the forwarding, SpinModel.call() is invoked without its required spin argument - so they are real regression tests.

Two comments inline: one blocking-grade, one weaker than it first looked.

Smaller notes, none blocking:

  1. spin = np.asarray(spin) inherits whatever dtype the caller supplied. pt builds the tensor with an explicit GLOBAL_PT_FLOAT_PRECISION, and deepmd/entrypoints/test.py registers spin with high_prec=False, so under DP_INTERFACE_PREC=low a float32 spin would meet float64 coords here.

  2. model_kwargs.update(box=..., fparam=..., aparam=..., do_atomic_virial=...) silently discards a caller-supplied value for any of those four keys. "Evaluator-owned arguments take precedence" is a defensible policy, but raising on the collision would beat dropping it without a diagnostic.

  3. The _get_request_defs docstring still says the non-atomic path requests "only energy (tensor), force, and virial", which the new clause contradicts.

  4. No test covers atomic=True, the wrong-size ValueError branch, or a non-spin model receiving extra kwargs - the last of which is what the first inline comment is about.

  5. deepmd/jax/infer/deep_eval.py has both of these defects unfixed: it pops charge_spin but never spin, and its non-atomic category tuple also omits OUT. Out of scope here, but worth a follow-up issue so the JAX spin path does not stay silently broken.

natoms, numb_test = self._get_natoms_and_nframes(
coords, atom_types, len(atom_types.shape) > 1
)
model_kwargs = dict(kwargs)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forwarding kwargs wholesale into model(...) regresses dp test for every dpmodel model, spin or not.

deepmd/entrypoints/test.py always passes the full set, whether or not the model supports any of it:

ret = dp.eval(coord, box, atype, fparam=fparam, aparam=aparam,
              atomic=has_atom_ener, efield=efield, mixed_type=mixed_type,
              spin=spin, charge_spin=charge_spin)

DeepPot.eval consumes mixed_type and relays the rest, so {efield: None, spin: None, charge_spin: None} arrives here, lands in model_kwargs, and reaches model.call(). Neither EnergyModel.call nor SpinModel.call declares efield, neither has a **kwargs, and NativeOP.__call__ forwards unfiltered.

I ran both against this branch's forwarding:

deeppot_dpa.yaml       -> TypeError: EnergyModel.call() got an unexpected keyword argument 'efield'
deeppot_dpa_spin.yaml  -> TypeError: SpinModel.call() got an unexpected keyword argument 'efield'

and confirmed the identical call succeeds on master, where the kwargs are dropped:

dp.eval(coord, box, atype, efield=None, mixed_type=False, spin=None, charge_spin=None)
-> [(1, 1), (1, 6, 3), (1, 9)]

So a working path becomes a hard failure for non-spin models, and dp test on a dpmodel spin model - the workflow #5661 is about - still does not work after this fix, just with a different exception. Nothing in CI drives dp test against a dpmodel model, which is why it stays green.

Worth checking against #5884, which is landing on these same signatures: it adds spin: Array | None = None to call_common and call_common_lower_graph, so spin=None would stop being fatal once that merges - but it adds efield nowhere (no added line in that whole diff mentions it), so this failure survives it unchanged. That PR is also widening the accepted-kwarg set per model class and per route, which is an argument against betting on the signature surface at all.

Every sibling backend extracts by name instead: pt pulls spin out explicitly, jax does kwargs.pop("charge_spin", None), pt_expt uses kwargs.get("spin"), and efield is TF-only by design. Taking spin and charge_spin by name and ignoring the rest fixes both cases here. A test that evaluates a plain model with the dp test kwarg set would lock it in.

)
# DeepPot always returns the magnetic-atom mask for spin
# models, even when per-atom energy was not requested.
or x.name == "mask_mag"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flagging this as a question rather than a defect, because the evidence cuts both ways.

The concern: pt and pd get mask_mag at atomic=False by including OutputVariableCategory.OUT in the non-atomic filter, so they need no per-name clause. dpmodel and jax omit OUT:

pt       OUT, REDU, DERV_R, DERV_C_REDU, DERV_R_DERV_R
pd       OUT, REDU, DERV_R, DERV_C_REDU, DERV_R_DERV_R
dpmodel       REDU, DERV_R, DERV_C_REDU
jax           REDU, DERV_R, DERV_C_REDU, DERV_R_DERV_R

Matching one output by name leaves that divergence in place: dpmodel's atomic=False result still omits energy (atom energy) and mask, which pt and pd return, so a consumer written against pt still sees a different dict here.

What argues the other way: #5884 hits the same wall independently and resolves it the same way, in deepmd/pt_expt/infer/deep_eval.py, with a comment conceding the point:

# ... Category alone [is insufficient] ... ``mask_mag`` is exported directly by the graph
if odef.name == "mask_mag":
    return "mask_mag"

Two unrelated changes both reaching for a name check on mask_mag suggests the field genuinely does not fit the category scheme, rather than dpmodel being careless. So I am not asking for the tuple to be aligned blindly - adding OUT also starts requesting atom energy at atomic=False, which is a real cost.

What I would like is for the choice to be explicit: either align the tuple with pt/pd, or leave the name clause and extend the comment to say why mask_mag is not category-addressable, so the next OUT-category output does not get a second name clause by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Code scan] Forward spin inputs through the dpmodel DeepEval backend

3 participants