feat: add charge spin to cpp runtime#5509
Open
anyangml wants to merge 23 commits into
Open
Conversation
for more information, see https://pre-commit.ci
Contributor
|
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:
📝 WalkthroughWalkthroughThreads optional per-call charge_spin through DeepPot backend and public compute APIs, PT/PTExpt runtimes, C API and deepmd.hpp bridge, LAMMPS PairDeep wiring, metadata export, and adds generators/tests exercising explicit vs default charge_spin and nlist paths. ChangesCharge/Spin Conditioning Feature Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds support for passing a runtime charge/spin conditioning vector from the LAMMPS pair style and C++ API down into the .pt2 (PTExpt) inference backend, instead of relying solely on defaults embedded in model metadata.
Changes:
- Extend LAMMPS
pair_style deepmdparsing to acceptcharge_spinandcharge_spin_from_compute, and thread the resulting vector intoDeepPot::compute(...). - Extend the C++
DeepPot/DeepPotBackendinterfaces and the PTExpt backend to accept and consume a runtimecharge_spininput with fallback to metadata defaults. - Add PTExpt
.pt2runtime tensor construction forcharge_spin, plus new computew overloads that carry it through.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| source/lmp/pair_deepmd.cpp | Adds new pair_style keys for charge/spin and passes charge_spin into runtime inference calls. |
| source/lmp/pair_base.h | Adds storage/flags and a helper to populate charge_spin from a LAMMPS compute. |
| source/lmp/pair_base.cpp | Implements make_charge_spin_from_compute and initializes the new compute flag. |
| source/api_cc/src/DeepPotPTExpt.cc | Threads charge_spin into .pt2 model invocation and adds runtime/default tensor construction. |
| source/api_cc/src/DeepPot.cc | Extends DeepPot::compute overloads to accept charge_spin; adds dim_chg_spin() forwarding. |
| source/api_cc/include/DeepPotPTExpt.h | Declares PTExpt charge_spin-aware overloads and exposes dim_chg_spin(). |
| source/api_cc/include/DeepPot.h | Adds dim_chg_spin() to backends and provides default charge_spin-aware computew overloads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
599
to
602
| dim_fparam = deep_pot_model_devi.dim_fparam(); | ||
| dim_aparam = deep_pot_model_devi.dim_aparam(); | ||
| dim_chg_spin = deep_pot.dim_chg_spin(); | ||
| assert(cutoff == deep_pot.cutoff() * dist_unit_cvt_factor); |
Comment on lines
+515
to
+522
| auto dbl_options = torch::TensorOptions().dtype(torch::kFloat64); | ||
| if (!charge_spin.empty()) { | ||
| charge_spin_tensor = | ||
| torch::from_blob(const_cast<double*>(charge_spin.data()), | ||
| {1, static_cast<std::int64_t>(charge_spin.size())}, | ||
| dbl_options) | ||
| .clone() | ||
| .to(device); |
Comment on lines
+839
to
+846
| auto dbl_options = torch::TensorOptions().dtype(torch::kFloat64); | ||
| if (!charge_spin.empty()) { | ||
| charge_spin_tensor = | ||
| torch::from_blob(const_cast<double*>(charge_spin.data()), | ||
| {1, static_cast<std::int64_t>(charge_spin.size())}, | ||
| dbl_options) | ||
| .clone() | ||
| .to(device); |
Comment on lines
+962
to
+967
| std::vector<double> frame_chg_spin; | ||
| if (!charge_spin.empty()) { | ||
| size_t s_dcsp = static_cast<size_t>(dchgspin); | ||
| frame_chg_spin.assign(charge_spin.begin() + s_ff * s_dcsp, | ||
| charge_spin.begin() + (s_ff + 1) * s_dcsp); | ||
| } |
Comment on lines
+1139
to
+1144
| std::vector<double> frame_chg_spin; | ||
| if (!charge_spin.empty()) { | ||
| size_t s_dcsp = static_cast<size_t>(dchgspin); | ||
| frame_chg_spin.assign(charge_spin.begin() + s_ff * s_dcsp, | ||
| charge_spin.begin() + (s_ff + 1) * s_dcsp); | ||
| } |
Comment on lines
+511
to
+514
| // Build charge_spin tensor: use runtime value when provided, fall back to | ||
| // default_chg_spin_ stored in the .pt2 metadata. | ||
| at::Tensor charge_spin_tensor; | ||
| if (dchgspin > 0) { |
Comment on lines
+699
to
+704
| } else if (string(arg[iarg]) == string("charge_spin")) { | ||
| for (int ii = 0; ii < dim_chg_spin; ++ii) { | ||
| if (iarg + 1 + ii >= narg || is_key(arg[iarg + 1 + ii])) { | ||
| char tmp[1024]; | ||
| sprintf(tmp, "Illegal charge_spin, the dimension should be %d", | ||
| dim_chg_spin); |
Comment on lines
251
to
+256
| if (single_model || multi_models_no_mod_devi) { | ||
| // cvflag_atom is the right flag for the cvatom matrix | ||
| if (!(eflag_atom || cvflag_atom)) { | ||
| try { | ||
| deep_pot.compute(dener, dforce, dvirial, dcoord, dtype, dbox, nghost, | ||
| lmp_list, ago, fparam, daparam); | ||
| lmp_list, ago, fparam, daparam, charge_spin); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/lmp/pair_deepmd.cpp (1)
318-320:⚠️ Potential issue | 🟠 MajorEnsure multi-model deviation propagates
charge_spin(or warns when unsupported)In
source/lmp/pair_deepmd.cpp, the multi-model deviation callsdeep_pot_model_devi.compute(...)at lines 318-320 and 326-329 withoutcharge_spin, unlike the single-model path which passescharge_spintodeep_pot.compute(). TheDeepBaseModelDeviAPI insource/api_c/include/deepmd.hppdoes not provide acompute(...charge_spin...)-style overload (nocompute.*charge_spinoccurrences), so deviation output cannot be consistent for models withdim_chg_spin > 0unless the API/caller is extended. Add a warning/error whendim_chg_spin > 0and multi-model deviation mode is enabled, or implement a charge_spin-aware deviation compute.🤖 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/lmp/pair_deepmd.cpp` around lines 318 - 320, The multi-model deviation path calls deep_pot_model_devi.compute(...) without passing charge_spin while the single-model path uses deep_pot.compute(..., charge_spin...), so when dim_chg_spin > 0 the deviation results are inconsistent; update the caller in source/lmp/pair_deepmd.cpp to detect dim_chg_spin > 0 when multi-model deviation (DeepBaseModelDevi) is active and either (A) emit a clear warning/error that charge_spin is unsupported for deviation mode (include dim_chg_spin and mode name in message) or (B) extend the deviation codepath to accept and forward charge_spin to DeepBaseModelDevi (implement a new compute overload or adapter in DeepBaseModelDevi and call it from deep_pot_model_devi.compute); reference deep_pot_model_devi.compute, deep_pot.compute, DeepBaseModelDevi, dim_chg_spin, and charge_spin when making the change.
🤖 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/lmp/pair_deepmd.cpp`:
- Line 601: In the multi-model initialization path, change the source of
dim_chg_spin to be read from deep_pot_model_devi (like cutoff, numb_types,
numb_types_spin, dim_fparam, dim_aparam) and add an assertion that
deep_pot.dim_chg_spin() equals the value from deep_pot_model_devi; specifically,
replace or supplement the current direct read dim_chg_spin =
deep_pot.dim_chg_spin() with reading dim_chg_spin from deep_pot_model_devi
(e.g., dim_chg_spin = deep_pot_model_devi.dim_chg_spin()) and add the same
consistency check/assert between deep_pot and deep_pot_model_devi.
---
Outside diff comments:
In `@source/lmp/pair_deepmd.cpp`:
- Around line 318-320: The multi-model deviation path calls
deep_pot_model_devi.compute(...) without passing charge_spin while the
single-model path uses deep_pot.compute(..., charge_spin...), so when
dim_chg_spin > 0 the deviation results are inconsistent; update the caller in
source/lmp/pair_deepmd.cpp to detect dim_chg_spin > 0 when multi-model deviation
(DeepBaseModelDevi) is active and either (A) emit a clear warning/error that
charge_spin is unsupported for deviation mode (include dim_chg_spin and mode
name in message) or (B) extend the deviation codepath to accept and forward
charge_spin to DeepBaseModelDevi (implement a new compute overload or adapter in
DeepBaseModelDevi and call it from deep_pot_model_devi.compute); reference
deep_pot_model_devi.compute, deep_pot.compute, DeepBaseModelDevi, dim_chg_spin,
and charge_spin when making the change.
🪄 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: 173dd256-e998-4ff9-9db5-ca8571420859
📒 Files selected for processing (7)
source/api_cc/include/DeepPot.hsource/api_cc/include/DeepPotPTExpt.hsource/api_cc/src/DeepPot.ccsource/api_cc/src/DeepPotPTExpt.ccsource/lmp/pair_base.cppsource/lmp/pair_base.hsource/lmp/pair_deepmd.cpp
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #5509 +/- ##
==========================================
+ Coverage 81.42% 81.45% +0.03%
==========================================
Files 871 874 +3
Lines 96951 98524 +1573
Branches 4241 4308 +67
==========================================
+ Hits 78941 80252 +1311
- Misses 16708 16933 +225
- Partials 1302 1339 +37 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
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/infer/gen_chg_spin.py`:
- Around line 3-9: The docstring's explicit charge_spin example is out of sync
with the actual value used in the script: update the docstring text that
currently shows "[0.5, 0.8]" to match the script's actual explicit charge_spin
"[1.0, 2.0]" so the described [explicit] section matches the value used when
writing chg_spin.expected (the variable/parameter named charge_spin in this test
generator).
🪄 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: 2de1ac02-e00d-42aa-93c7-625581682ce7
📒 Files selected for processing (7)
deepmd/pt_expt/utils/serialization.pysource/api_c/include/deepmd.hppsource/api_cc/tests/test_deeppot_chg_spin_ptexpt.ccsource/install/test_cc_local.shsource/lmp/pair_base.cppsource/tests/infer/gen_chg_spin.pysource/tests/pt_expt/infer/test_deep_eval.py
🚧 Files skipped from review as they are similar to previous changes (1)
- source/lmp/pair_base.cpp
for more information, see https://pre-commit.ci
iProzd
requested changes
Jun 10, 2026
iProzd
left a comment
iProzd
left a comment
Member
There was a problem hiding this comment.
The following should be completed for the main runtime paths before merge:
-
Please wire
charge_spinthrough the C API /deepmd.hpppath.
In common LAMMPS builds withDP_USING_C_API,PairDeepMDusesdeepmd::hpp::DeepPotfromsource/api_c/include/deepmd.hpp. The newcharge_spinargument is currently accepted there but discarded via(void)charge_spin, socharge_spinhas no effect on this widely used path. Please add proper C API entry points insource/api_c/include/c_api.h/source/api_c/src/c_api.ccand havedeepmd.hppforwardcharge_spinthrough to the underlyingapi_cc::DeepPotimplementation. -
Please support the regular PyTorch
.pthbackend as well.
DeepPotPTcurrently does not override the newcharge_spin-awarecomputewoverloads, so.pthmodels fall back to the defaultDeepPotBackendimplementation and silently ignorecharge_spin. If runtimecharge_spinis a C++ runtime feature, it should also be implemented for the PT backend, not onlyDeepPotPTExpt/.pt2. -
Please also thread
charge_spinthrough model-deviation.
LAMMPS model-deviation currently callsdeep_pot_model_devi.compute(..., fparam, aparam)withoutcharge_spin, so the main model path and deviation path can use different effective conditioning.
Without these, charge_spin support is incomplete and can silently no-op in common usage.
DeepPotPT silently dropped runtime charge_spin: it never read the model's charge/spin dimension and never passed charge_spin to forward/forward_lower, so .pth charge_spin models (DPA3 add_chg_spin_ebd) fell back to the stored default_chg_spin. This also affected the model-deviation path when its sub-models are .pth. - DeepPotPT: override dim_chg_spin(); read dim_chg_spin / default_chg_spin from the jit-exported model methods (guarded with find_method for old .pth without them); build the charge_spin tensor (runtime value or default fallback) and thread it into forward (no-nlist) and forward_lower (nlist), only when the model has a charge/spin embedding so other models are unaffected. For the non-message-passing edge case a real None comm_dict is passed (an empty Dict would wrongly flip parallel_mode). - gen_chg_spin.py: also export chg_spin.pth and verify it reproduces the .pt2 reference (PBC + NoPbc); add NoPbc reference sections. - Add test_deeppot_chg_spin_pt.cc mirroring the DPA3 .pth layout (PBC standalone + NoPbc lmp_nlist), covering explicit charge_spin and the default fallback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- pair_base.cpp: guard make_charge_spin_from_compute against modify->find_compute() returning -1 (out-of-bounds read before the null check), matching the find_compute contract. - DeepPotModelDevi: add dim_chg_spin() (api_cc delegates to the first model; C-API returns 0). pair_deepmd now reads dim_chg_spin from deep_pot_model_devi in the multi-model branch and asserts it matches deep_pot, consistent with the other dims. - DeepPotPTExpt: validate runtime charge_spin size against dim_chg_spin. Single-frame paths require exactly dim_chg_spin values; the multi-frame (nframes / mixed_type) paths accept either a single dim_chg_spin vector (now broadcast to all frames) or nframes * dim_chg_spin, and throw on any other length instead of slicing out of range. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
for more information, see https://pre-commit.ci
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/api_cc/include/DeepPotPT.h`:
- Around line 343-397: DeepPotPT currently declares computew_mixed_type
overloads without the charge_spin parameter which hides DeepPotBackend's
charge-spin-aware overloads; fix this by either adding a using declaration (e.g.
using DeepPotBackend::computew_mixed_type;) inside class DeepPotPT or by
redeclaring/overriding the charge-spin variants (functions named
computew_mixed_type taking const std::vector<double>& charge_spin) in DeepPotPT
so callers through the DeepPotPT static type can access the charge_spin-aware
overloads.
In `@source/api_cc/src/DeepPotPT.cc`:
- Around line 264-289: In DeepPotPT (around the charge_spin_tensor construction)
validate that when dchgspin > 0 the runtime vector charge_spin, if non-empty,
has size == dchgspin and likewise default_chg_spin_ (when used) has size >=
dchgspin; if either size mismatches, throw a deepmd::deepmd_exception with a
clear message instead of proceeding to torch::from_blob; mirror this same guard
in the non-neighbor-list path that also constructs charge_spin tensors so both
code paths consistently reject mismatched lengths before calling from_blob or
cloning.
In `@source/api_cc/tests/test_deeppot_chg_spin_pt.cc`:
- Around line 66-83: The SetUp method currently calls dp.init(kModelPath)
unconditionally causing hard failures if the chg_spin.pth model file is missing;
change SetUp to check for the existence/readability of kModelPath (the generated
.pth) before calling dp.init, and call GTEST_SKIP() with a clear message when
the file is absent so tests are skipped instead of failing; apply the same
existence check and skip logic to the other fixture's SetUp (the one around
lines 185-204) that also uses kModelPath/dp.init to ensure both fixtures
gracefully skip when the model generation was skipped.
In `@source/tests/infer/gen_chg_spin.py`:
- Around line 90-99: The export block using pt_deserialize_to_file can fail but
the later os.path.exists(pth_path) parity check will still pick up a
pre-existing stale chg_spin.pth; modify the export logic to track success (e.g.,
set a boolean like pth_exported = True only if pt_deserialize_to_file succeeds)
or remove any existing pth_path on exception, and then gate the subsequent
validation by checking that pth_exported is True (or that the fresh file was
just written) before running the parity validation; update references in this
file to pth_path and pt_deserialize_to_file accordingly so validation is skipped
when export fails.
🪄 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: f43bfbe2-e39d-465d-aef7-d8a1e4a0420a
📒 Files selected for processing (9)
source/api_c/include/deepmd.hppsource/api_cc/include/DeepPot.hsource/api_cc/include/DeepPotPT.hsource/api_cc/src/DeepPotPT.ccsource/api_cc/src/DeepPotPTExpt.ccsource/api_cc/tests/test_deeppot_chg_spin_pt.ccsource/lmp/pair_base.cppsource/lmp/pair_deepmd.cppsource/tests/infer/gen_chg_spin.py
🚧 Files skipped from review as they are similar to previous changes (4)
- source/lmp/pair_base.cpp
- source/api_c/include/deepmd.hpp
- source/lmp/pair_deepmd.cpp
- source/api_cc/src/DeepPotPTExpt.cc
Address iProzd's review: Point 1 — charge_spin had no effect on the common LAMMPS build (DP_USING_C_API), which uses deepmd::hpp::DeepPot. It was accepted there but discarded via (void)charge_spin. Now: - Add C API entry points (DP_C_API_VERSION 26->27): DP_DeepPotGetDimChgSpin and the version-3 compute functions DP_DeepPotCompute3 / *f3 / ComputeNList3 / *f3, which take a per-frame charge_spin (nframes x dim_chg_spin, always double) and forward it to api_cc::DeepPot::compute. The internal variant helpers gained an optional charge_spin arg, so the existing version-2 functions are unchanged. - deepmd.hpp DeepPot now reads dim_chg_spin via DP_DeepPotGetDimChgSpin (dim_chg_spin() returns it) and threads charge_spin through to the version-3 entry points. When charge_spin is empty / the model has no charge-spin embedding, it keeps the version-2 path so non-charge_spin models still work against an older libdeepmd_c. Inline comment — remove the charge_spin_from_compute pair_style keyword (and make_charge_spin_from_compute): charge_spin is a per-frame global physical input and is set statically via the charge_spin keyword; deriving it from a per-step compute was unnecessary. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
for more information, see https://pre-commit.ci
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/api_c/include/deepmd.hpp (1)
2197-2200:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject
charge_spinon model-deviation calls instead of silently ignoring it.These overloads now accept
charge_spin, but(void)charge_spindrops it and returns unconditioned results. Since this path reportsdim_chg_spin() == 0, failing fast on non-empty input is safer than pretending the argument was applied.Suggested fix
- // charge_spin is not supported via the C-API model-deviation path. - (void)charge_spin; + if (!charge_spin.empty()) { + throw deepmd::hpp::deepmd_exception( + "charge_spin is not supported by DeepPotModelDevi via the C API"); + }🤖 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/api_c/include/deepmd.hpp` around lines 2197 - 2200, The model-deviation C-API overload that currently takes const std::vector<double>& charge_spin = std::vector<double>() must not silently ignore non-empty charge_spin; instead check if charge_spin.empty() and if not return/raise an error (e.g., throw std::invalid_argument or use the C-API error reporting mechanism) with a clear message like "charge_spin not supported for model-deviation"; remove the (void)charge_spin no-op and ensure the code also documents/returns failure when dim_chg_spin() == 0 and a non-empty charge_spin is provided so callers fail fast.
🤖 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/api_c/include/deepmd.hpp`:
- Around line 1167-1174: Validate charge_spin before passing its raw pointer
into _DP_DeepPotCompute: add a single helper (e.g. check_charge_spin_or_ptr /
validate_and_get_charge_spin_ptr) used by all compute overloads that build
charge_spin__; if dchgspin > 0 require charge_spin.size() == nframes * natoms *
dchgspin and return charge_spin.data() (or throw/return an error if sizes
mismatch), and if dchgspin == 0 ensure any non-empty charge_spin is rejected
(throw or return nullptr with an explicit error) so you never hand an
incorrectly sized or silently dropped buffer to _DP_DeepPotCompute.
- Around line 1111-1115: DeepPot's member dchgspin is never initialized and
dim_chg_spin() exposes it without guarding; initialize dchgspin in DeepPot
constructors (set to 0 in the default/member initializer and assign correctly in
any other constructor paths) and add an initialization guard in the getter (e.g.
assert or throw if the object is not initialized) so dim_chg_spin() cannot
return garbage; update the DeepPot constructors and the method dim_chg_spin() to
reference the existing initialization flag (or add one) and ensure dchgspin is
set before being returned.
---
Outside diff comments:
In `@source/api_c/include/deepmd.hpp`:
- Around line 2197-2200: The model-deviation C-API overload that currently takes
const std::vector<double>& charge_spin = std::vector<double>() must not silently
ignore non-empty charge_spin; instead check if charge_spin.empty() and if not
return/raise an error (e.g., throw std::invalid_argument or use the C-API error
reporting mechanism) with a clear message like "charge_spin not supported for
model-deviation"; remove the (void)charge_spin no-op and ensure the code also
documents/returns failure when dim_chg_spin() == 0 and a non-empty charge_spin
is provided so callers fail fast.
🪄 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: 72380d4b-c7bf-4f1a-a158-1cee3bdef477
📒 Files selected for processing (6)
source/api_c/include/c_api.hsource/api_c/include/deepmd.hppsource/api_c/src/c_api.ccsource/lmp/pair_base.cppsource/lmp/pair_base.hsource/lmp/pair_deepmd.cpp
💤 Files with no reviewable changes (3)
- source/lmp/pair_base.h
- source/lmp/pair_base.cpp
- source/lmp/pair_deepmd.cpp
Completes the C API charge_spin support so model-deviation in the common LAMMPS build (DP_USING_C_API) no longer silently drops charge_spin: - Add C API entry points: DP_DeepPotModelDeviGetDimChgSpin and the version-3 model-deviation compute functions DP_DeepPotModelDeviCompute3 / *f3 / ComputeNList3 / *f3, forwarding the per-frame charge_spin to api_cc::DeepPotModelDevi::compute. The internal model-devi variant helpers gained an optional charge_spin arg, so the existing version-2 functions are unchanged. - deepmd.hpp DeepPotModelDevi now reads dim_chg_spin via DP_DeepPotModelDeviGetDimChgSpin and threads charge_spin through to the version-3 entry points; empty charge_spin / non-charge_spin models keep the version-2 path for compatibility with an older libdeepmd_c. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- DeepPotPT.h: add `using DeepPotBackend::computew[_mixed_type]` so the base charge_spin-aware overloads are not hidden by the non-charge_spin declarations. - DeepPotPT.cc: validate runtime/default charge_spin size against dim_chg_spin before building tensors (both nlist and no-nlist paths), avoiding out-of-range reads / opaque TorchScript failures. - deepmd.hpp: initialize dchgspin in DeepPot/DeepPotModelDevi constructors and guard dim_chg_spin() with assert(dp); add a shared validate_charge_spin() helper (rejects wrong size or charge_spin on a non-charge_spin model) and use it across all compute overloads. - test_deeppot_chg_spin_pt.cc: GTEST_SKIP when chg_spin.pth is absent instead of hard-failing SetUp. - gen_chg_spin.py: remove any stale chg_spin.pth before export and gate the parity check on a pth_generated flag, not os.path.exists. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Exercises parsing + wiring of the `charge_spin` keyword end-to-end against chg_spin.pt2 (DPA3, generated by gen_chg_spin.py): the default path (no keyword -> stored default_chg_spin) and an explicit `charge_spin 1.0 2.0`, both checked against the gen-script reference, plus a guard that an explicit charge_spin actually changes the energy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
for more information, see https://pre-commit.ci
The .pth (pt) DeepEval does not expose get_dim_chg_spin (only the .pt2 / pt_expt one does), so the .pth parity assert raised AttributeError and crashed gen_chg_spin.py. Under test_cc_local.sh's `set -e`, that aborted the script before ctest, failing the whole C++ job. The energy/force parity checks already validate that charge_spin is threaded for the .pth backend. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
iProzd
requested changes
Jun 11, 2026
Per review, charge_spin should follow the same convention as the other inputs (coord/fparam/aparam/spin): typed FPTYPE and grouped with the inputs (right after aparam), not a trailing double after the outputs. - c_api.h / c_api.cc: the version-3 entry points now take charge_spin as FPTYPE (double in the *3 funcs, float in the *f3 funcs), positioned after aparam. The internal *_variant helpers take const VALUETYPE* charge_spin and convert to std::vector<double> for the api_cc::DeepPot interface (which stores charge_spin as float64). api_cc and the compute backends are unchanged. - deepmd.hpp: the _DP_* helper templates take const FPTYPE* charge_spin after aparam; the public DeepPot/DeepPotModelDevi::compute overloads take const std::vector<VALUETYPE>& charge_spin; validate_charge_spin is now a template returning const FPTYPE*. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
for more information, see https://pre-commit.ci
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary by CodeRabbit
New Features
Tests