Skip to content

Global derivative migration#5498

Open
Lily200202 wants to merge 10 commits into
deepmodeling:masterfrom
Lily200202:global-derivative-migration
Open

Global derivative migration#5498
Lily200202 wants to merge 10 commits into
deepmodeling:masterfrom
Lily200202:global-derivative-migration

Conversation

@Lily200202

@Lily200202 Lily200202 commented Jun 6, 2026

Copy link
Copy Markdown

Summary

This pull request adds a DeepMD backend path for providing dE/dN to the generic dedn interface used by the companion LAMMPS fix uvt pull request.

The main additions are:

  • support for sourcing frame parameters from a LAMMPS fix (fparam_from_fix)
  • a new compute, deepmd/fparam/dedn, for evaluating the derivative of the DeepMD energy with respect to a chosen frame parameter
  • the corresponding plugin, API, documentation, and regression-test updates needed to support this path

This pull request is intended to make DeepMD act as a provider of dE/dN, while the LAMMPS-side constant-potential dynamics remains generic and backend-independent.

Related Issue(s)

None currently referenced.

Author(s)

Li Fu
Peking University, Beijing 100871, People’s Republic of China
Corresponding e-mail: fuli200202@163.com

Licensing

By submitting this pull request, I confirm that I am authorized to contribute these changes under the license of this project.

Artificial Intelligence (AI) Tools Usage

By submitting this pull request, I confirm that I used AI tools to help draft and revise parts of the implementation, documentation, tests, and validation workflow. In particular, OpenAI Codex / ChatGPT were used to assist with code drafting, documentation wording, test scaffolding, and iterative implementation review. Final design choices, validation decisions, and repository integration were reviewed and approved by the human author.

Backward Compatibility

This pull request is intended to be backward compatible for existing DeepMD/LAMMPS workflows.

The new functionality is opt-in:

  • fparam_from_fix
  • compute deepmd/fparam/dedn

Existing pair-style usage without these additions is intended to keep its previous behavior.

Implementation Notes

This pull request adds a provider-side path for constant-potential simulations where a LAMMPS fix carries a generalized coordinate, and DeepMD evaluates the corresponding energy derivative with respect to that coordinate.

The intended workflow is:

  1. A LAMMPS fix carries the generalized coordinate (e.g., Ne from fix uvt)
  2. DeepMD reads that coordinate as a frame parameter via fparam_from_fix
  3. Compute deepmd/fparam/dedn evaluates dE/dN
  4. The resulting scalar is consumed by the generic dedn interface on the LAMMPS side

In schematic form:


fix -> frame parameter -> DeepMD energy derivative -> dedn consumer

This pull request includes:

  • fparam_from_fix support in the LAMMPS DeepMD pair path
  • a new compute deepmd/fparam/dedn
  • plugin registration updates
  • API updates needed to expose the derivative path
  • documentation updates
  • an automated regression test for the new provider chain

The deepmd/fparam/dedn compute supports sources of the form:

  • v_name
  • c_ID
  • f_ID

Vector-valued sources may use the usual [index] syntax.

This pull request focuses only on the DeepMD provider side. It does not introduce the constant-potential time integrator itself; that part is handled in the companion LAMMPS pull request.

Heavy realistic validation assets are intentionally not included in this pull request to keep the review scope focused on the provider interface, API path, and regression coverage.

Validation

Validation for this pull request includes:

  • regression coverage for fparam_from_fix
  • regression coverage for compute deepmd/fparam/dedn
  • verification that the new provider chain can be consumed by the generic constant-potential framework

This provider path was also exercised on realistic constant-potential validation systems outside the minimal automated regression test, but those heavier validation assets are intentionally kept out of this pull request to keep the review scope manageable.

Relation to the Companion PR

This pull request is designed to pair with the companion LAMMPS pull request that adds fix uvt.

  • The LAMMPS pull request provides the generic constant-potential consumer
  • This pull request provides a DeepMD backend that supplies dE/dN
  • Keeping them separate makes the framework/backend boundary explicit and keeps review scope manageable in both repositories

Post Submission Checklist

  • The feature or features in this pull request is complete
  • Licensing information is complete
  • Corresponding author information is complete
  • Suitable documentation updates are included
  • Suitable regression tests are included
  • The new functionality has been verified in local testing
  • The new functionality is intended to be backward compatible for existing workflows

Further Information, Files, and Links

This pull request is intentionally separated from the companion LAMMPS pull request so that the generic constant-potential framework and the backend-specific provider can be reviewed independently.

Summary by CodeRabbit

  • New Features

    • Source frame parameters from LAMMPS fixes (fparam_from_fix).
    • New compute to report dE/dN (direct or finite-difference) alongside energy, forces, and virials.
    • API/backend and plugin support to return/query dE/dN from model evaluations.
  • Documentation

    • Updated docs with fparam_from_fix usage, examples, supported source forms, and compute syntax for finite-difference derivatives.
  • Tests

    • Added tests validating fparam_from_fix behavior and the dE/dN compute.

Comment thread source/api_cc/src/DeepPotTF.cc Fixed
Comment thread source/api_cc/src/DeepPotTF.cc Fixed
@codecov

codecov Bot commented Jun 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 27.01525% with 335 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.16%. Comparing base (e05af21) to head (36243f0).

Files with missing lines Patch % Lines
source/api_cc/src/DeepPotTF.cc 4.00% 96 Missing ⚠️
source/api_c/src/c_api.cc 0.00% 90 Missing ⚠️
source/lmp/compute_deepmd_fparam_dedn.cpp 33.33% 55 Missing and 19 partials ⚠️
source/lmp/pair_deepmd.cpp 60.00% 28 Missing and 16 partials ⚠️
source/lmp/pair_base.cpp 29.62% 14 Missing and 5 partials ⚠️
source/api_cc/include/DeepPot.h 0.00% 6 Missing ⚠️
source/api_cc/src/DeepPot.cc 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5498      +/-   ##
==========================================
- Coverage   81.42%   81.16%   -0.26%     
==========================================
  Files         871      873       +2     
  Lines       96951    97401     +450     
  Branches     4241     4344     +103     
==========================================
+ Hits        78941    79057     +116     
- Misses      16708    17002     +294     
- Partials     1302     1342      +40     

☔ 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.

@Lily200202 Lily200202 marked this pull request as ready for review June 8, 2026 12:09
@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 3ef8dfa7-fb02-4f14-9781-698b06caa48e

📥 Commits

Reviewing files that changed from the base of the PR and between 43bf36e and 36243f0.

📒 Files selected for processing (3)
  • source/api_c/src/c_api.cc
  • source/lmp/compute_deepmd_fparam_dedn.cpp
  • source/lmp/tests/test_lammps_fparam_from_fix_dedn.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • source/lmp/compute_deepmd_fparam_dedn.cpp
  • source/lmp/tests/test_lammps_fparam_from_fix_dedn.py
  • source/api_c/src/c_api.cc

📝 Walkthrough

Walkthrough

This PR adds comprehensive support for computing energy derivatives with respect to frame parameters across the DeepMD stack. It introduces C/C++ API extensions to compute dE/dN outputs, implements TensorFlow backend inference for derivatives, and integrates LAMMPS pair and compute interfaces to enable frame-parameter sweeps and finite-difference derivative estimation via LAMMPS fixes.

Changes

dE/dN Computation and LAMMPS Integration

Layer / File(s) Summary
DeepPot backend interface and TensorFlow implementation
source/api_cc/include/DeepPot.h, source/api_cc/include/DeepPotTF.h, source/api_cc/src/DeepPotTF.cc, source/api_cc/src/DeepPot.cc
Backend defines virtual computew_dedn methods for double/float variants; TensorFlow implementation runs TF session requesting dE/dN output alongside energy/force/virial, back-maps forces, and returns per-frame scalar results; DeepPot wrapper forwards to backend and extracts first-frame values.
C API function prototypes and implementations for dE/dN
source/api_c/include/c_api.h, source/api_c/src/c_api.cc
Adds four DP_DeepPotCompute*add function variants (double/float, basic/NList) with double* dE_dN output; implementations validate fparam constraints, compute via finite difference or direct backend call, and copy results to caller-provided C arrays.
C++ wrapper layer for neighbor-list computations
source/api_c/include/deepmd.hpp
Header-only template wrappers forward to C API *add functions; new DeepPot::compute overload for neighbor lists includes dE_dN reference output parameter.
Base pair infrastructure for fix-based frame parameters
source/lmp/pair_base.h, source/lmp/pair_base.cpp
Adds get_dim_fparam() accessor, make_fparam_from_fix() to populate fparam from LAMMPS Fix (scalar replication or indexed vector extraction), and state members do_fix_fparam/fix_fparam_id/fix_fparam_index.
PairDeepMD integration with fparam_from_fix and dE/dN caching
source/lmp/pair_deepmd.h, source/lmp/pair_deepmd.cpp
Extends settings() parsing to accept fparam_from_fix key with optional vector index; adds extract() hook for cached deepmd_dedn; introduces eval_energy_with_fparam() for parameter-override energy evaluation; invalidates cache across multiple compute branches.
LAMMPS compute deepmd/fparam/dedn for dE/dN derivatives
source/lmp/compute_deepmd_fparam_dedn.h, source/lmp/compute_deepmd_fparam_dedn.cpp
New compute class parses source (variable/compute/fix with optional vector index) and delta step; validates pair compatibility; returns direct deepmd_dedn from pair when available, otherwise computes central finite-difference derivative of pair energy.
Plugin registration and build configuration
source/lmp/plugin/deepmdplugin.cpp, source/lmp/plugin/CMakeLists.txt
Registers new deepmd/fparam/dedn compute plugin; enforces C++17 standard and updates CMake minimum version (3.23.0) for plugin compatibility.
Tests and documentation
doc/third-party/lammps-command.md, source/lmp/tests/test_lammps_fparam_from_fix_dedn.py
Pytest validates fparam from fix and dE/dN compute via PyLammps; documentation describes fparam_from_fix keyword, compute deepmd/fparam/dedn syntax, source forms, and delta parameter scope.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • njzjz
  • iProzd
  • wanghan-iapcm
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 23.68% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'Global derivative migration' is vague and does not clearly convey the primary changes in this pull request. Specify the main change more concretely, such as 'Add dE/dN derivative computation support for DeepMD frame parameters' or 'Implement fparam_from_fix and deepmd/fparam/dedn compute'.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
doc/third-party/lammps-command.md (1)

52-52: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add fparam_from_fix to the pair_style keyword synopsis.

Line [52] omits _fparam_from_fix_, but this keyword is documented below and implemented in source/lmp/pair_deepmd.cpp (settings parser). This makes the synopsis incomplete for users.

Suggested patch
-- keyword = _out_file_ or _out_freq_ or _fparam_ or _fparam_from_compute_ or _aparam_from_compute_ or _atomic_ or _relative_ or _relative_v_ or _aparam_ or _ttm_
+- keyword = _out_file_ or _out_freq_ or _fparam_ or _fparam_from_compute_ or _fparam_from_fix_ or _aparam_from_compute_ or _atomic_ or _relative_ or _relative_v_ or _aparam_ or _ttm_
🤖 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 `@doc/third-party/lammps-command.md` at line 52, Update the pair_style keyword
synopsis to include the missing `_fparam_from_fix_` token: locate the keyword
list in doc/third-party/lammps-command.md (the line containing "keyword =
_out_file_ or _out_freq_ ...") and add `_fparam_from_fix_` into that enumeration
so the doc matches the implementation in source/lmp/pair_deepmd.cpp (the
settings parser which documents and supports `_fparam_from_fix_`).
🧹 Nitpick comments (1)
source/lmp/tests/test_lammps_fparam_from_fix_dedn.py (1)

71-71: ⚡ Quick win

Make delta explicit in the compute command to avoid default-coupling.

The test assumes the compute default delta equals 1e-6. Passing delta explicitly keeps this robust if defaults change later.

Suggested patch
-def _lammps(fp_value, units="metal") -> PyLammps:
+def _lammps(fp_value, units="metal", dedn_delta=1.0e-6) -> PyLammps:
@@
-    lammps.compute("dedn all deepmd/fparam/dedn f_fpfix[2]")
+    lammps.compute(f"dedn all deepmd/fparam/dedn f_fpfix[2] {dedn_delta}")

Also applies to: 97-102

🤖 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/tests/test_lammps_fparam_from_fix_dedn.py` at line 71, The compute
call using the DeepMD dedn compute (the string "dedn all deepmd/fparam/dedn
f_fpfix[2]" passed to lammps.compute) relies on the implicit default delta;
update this compute invocation (and the other similar calls around lines 97–102)
to pass delta explicitly (use delta 1e-6) so the test no longer depends on
LAMMPS default behavior.
🤖 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/src/c_api.cc`:
- Around line 693-699: The code currently fills atomic_energy and atomic_virial
with zeros which can silently produce wrong results for callers that requested
atomic outputs; instead, detect when either atomic_energy or atomic_virial is
non-null and fail fast (return an error/status or throw) if the backend overload
that computes per-atom outputs isn’t being used, or alternatively wire the real
per-atom outputs through the backend call that computes dE_dN; specifically,
replace the zero-fill behavior around atomic_energy and atomic_virial with a
guard that checks for non-null atomic_energy/atomic_virial and either (a)
invokes the backend variant that produces atomic outputs (dE_dN path) or (b)
returns a clear error/status indicating atomic outputs are unsupported for this
call (refer to atomic_energy, atomic_virial and the dE_dN-producing backend
overload to locate the relevant code).
- Around line 675-678: Replace the throw in the multi-frame check with the same
C-ABI error-path used elsewhere: inside the functions DP_DeepPotComputeNList2add
and DP_DeepPotComputeNListf2add, when nframes != 1 set dp->exception to a new
deepmd::deepmd_exception with the same message, ensure the function returns the
appropriate error sentinel (matching other C entry points) instead of throwing,
and let callers use DP_DeepPotCheckOK to inspect the stored dp->exception.

In `@source/lmp/compute_deepmd_fparam_dedn.cpp`:
- Around line 30-37: The current bracket parsing in
compute_deepmd_fparam_dedn.cpp uses atoi(...) - 1 which silently maps
non-numeric or zero values to -1 (falling back to scalar mode); change the logic
around token/lb/rb and source_index to validate that the substring
token.substr(lb+1, rb-lb-1) is non-empty and contains only digits, then parse it
with a safe conversion (e.g., std::stoi inside try/catch or manual digit-to-int
accumulation), ensure the resulting integer is >= 1, set source_index = parsed -
1, and call error->all(FLERR, "...") with a clear message if validation/parsing
fails instead of allowing -1.

In `@source/lmp/pair_base.cpp`:
- Around line 170-175: The code dereferences modify->fix[ifix] without checking
the find_fix result; update the logic in the block around
find_fix(fix_fparam_id) so you first check ifix (the return of modify->find_fix)
for -1 and call error->all with a clear message (including fix_fparam_id) before
attempting to index modify->fix; only after that guard should you assign Fix*
fix = modify->fix[ifix] and continue.

In `@source/lmp/pair_deepmd.cpp`:
- Around line 180-183: The fparam_override check only validates size but never
replaces the runtime fparam used in energy/force computations; update the code
paths that rebuild or set fparam (the blocks around where fparam is constructed
and passed into the compute calls referenced at lines 221-225 and 245-246) to,
when fparam_override.size() == static_cast<size_t>(dim_fparam), copy/assign
fparam_override into the fparam buffer/variable used for the compute and for the
+/- delta finite-difference variations; ensure you preserve the existing
size/type checks (dim_fparam, fparam vector) and use the same fparam variable
name so the compute calls and delta adjustments operate on the overridden
values.
- Around line 821-823: The parsing of fparam_from_fix currently does
atoi(arg[...] )-1 which can produce <=0 or accept non-numeric input silently;
update the argument handling in the parsing block that sets fix_fparam_index to
parse with strtol (use an endptr) and validate the token is a pure positive
integer >0 before subtracting 1, otherwise log an error (mentioning the bad
token) and fail fast (exit/throw) so invalid indices are rejected; reference the
variables/functions fix_fparam_index, arg[], is_key(...) and the
argument-parsing branch around fparam_from_fix.

In `@source/lmp/tests/test_lammps_fparam_from_fix_dedn.py`:
- Around line 91-94: The test_pair_fparam_from_fix currently compares
lammps.eval("pe") to _energy_at_fp(0.25852028) which recomputes energy via the
same fparam_from_fix path, making it tautological; update the test to assert
against an independent reference instead: either hard-code a precomputed energy
constant (computed offline) or compute the expected energy via an alternate,
independent LAMMPS setup (e.g., set explicit pair coefficients instead of using
fparam_from_fix) and assert lammps.eval("pe") equals that reference; modify
test_pair_fparam_from_fix and remove reliance on _energy_at_fp for the asserted
value so the test will catch regressions in fix-sourcing behavior.

---

Outside diff comments:
In `@doc/third-party/lammps-command.md`:
- Line 52: Update the pair_style keyword synopsis to include the missing
`_fparam_from_fix_` token: locate the keyword list in
doc/third-party/lammps-command.md (the line containing "keyword = _out_file_ or
_out_freq_ ...") and add `_fparam_from_fix_` into that enumeration so the doc
matches the implementation in source/lmp/pair_deepmd.cpp (the settings parser
which documents and supports `_fparam_from_fix_`).

---

Nitpick comments:
In `@source/lmp/tests/test_lammps_fparam_from_fix_dedn.py`:
- Line 71: The compute call using the DeepMD dedn compute (the string "dedn all
deepmd/fparam/dedn f_fpfix[2]" passed to lammps.compute) relies on the implicit
default delta; update this compute invocation (and the other similar calls
around lines 97–102) to pass delta explicitly (use delta 1e-6) so the test no
longer depends on LAMMPS default behavior.
🪄 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: df6c27d1-9d9a-4550-b901-f1f108cd8be1

📥 Commits

Reviewing files that changed from the base of the PR and between e05af21 and 8bde4e6.

📒 Files selected for processing (17)
  • doc/third-party/lammps-command.md
  • source/api_c/include/c_api.h
  • source/api_c/include/deepmd.hpp
  • source/api_c/src/c_api.cc
  • source/api_cc/include/DeepPot.h
  • source/api_cc/include/DeepPotTF.h
  • source/api_cc/src/DeepPot.cc
  • source/api_cc/src/DeepPotTF.cc
  • source/lmp/compute_deepmd_fparam_dedn.cpp
  • source/lmp/compute_deepmd_fparam_dedn.h
  • source/lmp/pair_base.cpp
  • source/lmp/pair_base.h
  • source/lmp/pair_deepmd.cpp
  • source/lmp/pair_deepmd.h
  • source/lmp/plugin/CMakeLists.txt
  • source/lmp/plugin/deepmdplugin.cpp
  • source/lmp/tests/test_lammps_fparam_from_fix_dedn.py

Comment thread source/api_c/src/c_api.cc
Comment thread source/api_c/src/c_api.cc Outdated
Comment on lines +744 to +811
template <typename VALUETYPE, typename ENERGYVTYPE>
void DeepPotTF::compute_dedn(ENERGYVTYPE& dener,
std::vector<VALUETYPE>& dforce_,
std::vector<VALUETYPE>& dvirial,
ENERGYTYPE& ddedn,
const std::vector<VALUETYPE>& dcoord_,
const std::vector<int>& datype_,
const std::vector<VALUETYPE>& dbox,
const int nghost,
const InputNlist& lmp_list,
const int& ago,
const std::vector<VALUETYPE>& fparam,
const std::vector<VALUETYPE>& aparam) {
int nall = datype_.size();
int nframes = nall > 0 ? (dcoord_.size() / nall / 3) : 1;
int nloc = nall - nghost;
std::vector<VALUETYPE> fparam_;
std::vector<VALUETYPE> aparam_;
validate_fparam_aparam(nframes, (aparam_nall ? nall : nloc), fparam, aparam);
tile_fparam_aparam(fparam_, nframes, dfparam, fparam);
tile_fparam_aparam(aparam_, nframes, (aparam_nall ? nall : nloc) * daparam,
aparam);

std::vector<std::pair<std::string, Tensor>> input_tensors;
std::vector<VALUETYPE> dcoord, dforce, aparam_sel;
std::vector<int> datype, fwd_map, bkw_map;
int nghost_real, nall_real, nloc_real;
select_real_atoms_coord(dcoord, datype, aparam_sel, nghost_real, fwd_map,
bkw_map, nall_real, nloc_real, dcoord_, datype_,
aparam_, nghost, ntypes, nframes, daparam, nall,
aparam_nall);

if (ago == 0) {
atommap = deepmd::AtomMap(datype.begin(), datype.begin() + nloc_real);
assert(nloc_real == atommap.get_type().size());

nlist_data.copy_from_nlist(lmp_list);
nlist_data.shuffle_exclude_empty(fwd_map);
nlist_data.shuffle(atommap);
nlist_data.make_inlist(nlist);
}

std::vector<ENERGYTYPE> dener_vec;
std::vector<ENERGYTYPE> ddedn_vec;
if (dtype == tensorflow::DT_DOUBLE) {
const int ret = session_input_tensors<double>(
input_tensors, dcoord, ntypes, datype, dbox, nlist, fparam_, aparam_sel,
atommap, nghost_real, ago, "", aparam_nall);
(void)ret;
assert(nloc_real == ret);
run_model_dedn<double>(dener_vec, dforce, dvirial, ddedn_vec, session,
input_tensors, atommap, nframes, nghost_real);
} else {
const int ret = session_input_tensors<float>(
input_tensors, dcoord, ntypes, datype, dbox, nlist, fparam_, aparam_sel,
atommap, nghost_real, ago, "", aparam_nall);
(void)ret;
assert(nloc_real == ret);
run_model_dedn<float>(dener_vec, dforce, dvirial, ddedn_vec, session,
input_tensors, atommap, nframes, nghost_real);
}

dforce_.resize(static_cast<size_t>(nframes) * fwd_map.size() * 3);
select_map<VALUETYPE>(dforce_, dforce, bkw_map, 3, nframes, fwd_map.size(),
nall_real);
dener = dener_vec[0];
ddedn = ddedn_vec[0];
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Preserve per-frame dE/dN outputs in the NList path.

run_model_dedn() already computes nframes energies and derivatives, but Lines 809-810 collapse both vectors to frame 0. That makes computew_dedn() return ener.size() == dedn.size() == 1 even when the input spans multiple frames, while force and virial still carry the full nframes payload. The backend contract in DeepPotBackend::computew_dedn(...) and the new DP_DeepPotCompute*2add(...) C APIs are frame-vector based, so multi-frame callers will silently lose every dE/dN value after the first frame.

Please mirror the existing scalar/vector compute(...) split here: keep a vector-returning dE/dN path internally, and only extract [0] in the single-frame wrapper that truly needs scalar outputs.

Comment thread source/lmp/compute_deepmd_fparam_dedn.cpp
Comment thread source/lmp/pair_base.cpp
Comment thread source/lmp/pair_deepmd.cpp
Comment thread source/lmp/pair_deepmd.cpp
Comment thread source/lmp/tests/test_lammps_fparam_from_fix_dedn.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 `@source/api_c/src/c_api.cc`:
- Around line 675-685: The NList dE/dN path must reject multi-component fparam
models: before calling DP_REQUIRES_OK(... dp->dp.compute(...)) check that
dp->dfparam == 1 and if not set dp->exception to a clear message (e.g. "Direct
dE/dN C API requires dfparam == 1 for fparam") and return; this mirrors the
check in DP_DeepPotCompute_variantadd and prevents ambiguous scalar dE_dN[0]
interpretation for dim_fparam > 1.

In `@source/lmp/compute_deepmd_fparam_dedn.cpp`:
- Around line 37-43: The parsing of the bracket index in ComputeDeepmdFparamDedn
currently only ensures one_based >= 1 but can overflow when casting to int;
before calling std::strtol set errno = 0, keep the existing endptr and one_based
< 1 checks, and then reject the value if errno == ERANGE or if one_based >
static_cast<long>(std::numeric_limits<int>::max()) + 1; only then safely assign
source_index = static_cast<int>(one_based - 1) and call error->all(FLERR, ...)
on these failure cases to prevent overflow/UB when converting one_based to
source_index.
🪄 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: fbd96d70-9c42-4ba7-9cf1-7586572e859c

📥 Commits

Reviewing files that changed from the base of the PR and between 8bde4e6 and 842a37b.

📒 Files selected for processing (5)
  • source/api_c/src/c_api.cc
  • source/lmp/compute_deepmd_fparam_dedn.cpp
  • source/lmp/pair_base.cpp
  • source/lmp/pair_deepmd.cpp
  • source/lmp/tests/test_lammps_fparam_from_fix_dedn.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • source/lmp/tests/test_lammps_fparam_from_fix_dedn.py
  • source/lmp/pair_deepmd.cpp

Comment thread source/api_c/src/c_api.cc
Comment thread source/lmp/compute_deepmd_fparam_dedn.cpp
@Lily200202 Lily200202 force-pushed the global-derivative-migration branch from d72a792 to 43bf36e Compare June 8, 2026 13:56

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
source/lmp/compute_deepmd_fparam_dedn.cpp (1)

70-80: 💤 Low value

Consider consistent validation for delta parsing.

Line 72 uses atof() which silently accepts malformed input like "1.0abc" (returning 1.0) and has undefined behavior on overflow. This is inconsistent with the careful strtol validation used for bracket indices at lines 41-48.

While the delta <= 0.0 check catches empty/non-numeric strings, using strtod with proper validation would be more robust:

Suggested improvement
   int iarg = 4;
   if (iarg < narg) {
-    delta = atof(arg[iarg]);
+    char* endptr = nullptr;
+    errno = 0;
+    delta = std::strtod(arg[iarg], &endptr);
+    if (endptr == arg[iarg] || *endptr != '\0' || errno == ERANGE) {
+      error->all(FLERR, "Invalid delta value in compute deepmd/fparam/dedn");
+    }
     ++iarg;
   }
🤖 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/compute_deepmd_fparam_dedn.cpp` around lines 70 - 80, Replace the
unsafe atof() parsing for delta with strtod()-based parsing that validates the
entire token and checks errno for overflow/underflow: use strtod(arg[iarg],
&endptr), ensure endptr points to the string terminator (no extra junk like
"1.0abc"), handle errno==ERANGE for range errors, assign the parsed value to
delta, then ++iarg; keep the existing iarg/narg bounds check and the delta <=
0.0 error->all(FLERR, ...) validation. Reference variables/functions: delta,
arg[], narg, iarg, error->all, FLERR (in
source/lmp/compute_deepmd_fparam_dedn.cpp).
🤖 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.

Nitpick comments:
In `@source/lmp/compute_deepmd_fparam_dedn.cpp`:
- Around line 70-80: Replace the unsafe atof() parsing for delta with
strtod()-based parsing that validates the entire token and checks errno for
overflow/underflow: use strtod(arg[iarg], &endptr), ensure endptr points to the
string terminator (no extra junk like "1.0abc"), handle errno==ERANGE for range
errors, assign the parsed value to delta, then ++iarg; keep the existing
iarg/narg bounds check and the delta <= 0.0 error->all(FLERR, ...) validation.
Reference variables/functions: delta, arg[], narg, iarg, error->all, FLERR (in
source/lmp/compute_deepmd_fparam_dedn.cpp).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: f2576aa3-9abe-4557-9f8c-807086f73b69

📥 Commits

Reviewing files that changed from the base of the PR and between d72a792 and 43bf36e.

📒 Files selected for processing (2)
  • source/api_c/src/c_api.cc
  • source/lmp/compute_deepmd_fparam_dedn.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • source/api_c/src/c_api.cc

@Lily200202 Lily200202 force-pushed the global-derivative-migration branch from 43bf36e to 36243f0 Compare June 8, 2026 15:02

@njzjz-bot njzjz-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for working on this. I think this needs another iteration before we add these public APIs and the LAMMPS compute. The main blocker is that the new derivative contract is not yet clear or consistently implemented: some paths use a direct TensorFlow output, while the documented LAMMPS compute and the non-nlist C path effectively use finite differences. That makes backend/model support, failure behavior, and API stability hard to reason about.

Please clarify whether this feature is meant to expose a direct model output or a finite-difference derivative, make the C/C++ API and LAMMPS compute follow the same contract, document backend/model limitations, and add tests for the C API/NList/direct-output and unsupported-model paths before merging.

— OpenClaw 2026.5.28 (model: custom-chat-jinzhezeng-group/gpt-5.5)

double* virial,
double* atomic_energy,
double* atomic_virial);
extern void DP_DeepPotCompute2add(DP_DeepPot* dp,

@njzjz-bot njzjz-bot Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This adds a new public C ABI entry point, but the name and contract are not clear enough for a stable API. From the implementation, DP_DeepPotCompute2add does not appear to mean the same thing as the NList variant: one path computes a finite-difference derivative while the TF/NList path fetches o_dE_dN directly. Please use an explicit name such as one mentioning Dedn/FparamDerivative, and document whether dE_dN is direct or finite-difference, the expected output size, supported dim_fparam, multi-frame behavior, and backend/model limitations.

std::vector<Tensor> output_tensors;
check_status(session->Run(
input_tensors,
{"o_energy", "o_force", "o_dE_dN", "o_atom_energy", "o_atom_virial"}, {},

@njzjz-bot njzjz-bot Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fetching o_dE_dN unconditionally makes this new NList/direct path require a model tensor that many existing TF models likely do not contain. Please add an explicit capability check or a clear error path, and cover the missing-o_dE_dN case in tests. This is especially important because the other newly added paths appear to use finite differences instead, so callers cannot infer this requirement from the API name.

double ComputeDeepmdFparamDedn::compute_scalar() {
invoked_scalar = update->ntimestep;
int dim = 0;
if (void* ptr = pair->extract("deepmd_dedn", dim)) {

@njzjz-bot njzjz-bot Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The documentation says this compute evaluates a finite-difference derivative, but this code first consumes a cached direct deepmd_dedn value if the pair style exposes one. Please decide which behavior is the contract. If direct output is intended, the docs/tests should say that this compute prefers direct model output and falls back to finite differences; if finite difference is intended, this direct fast path should be removed to avoid silently changing semantics later.

}

void* PairDeepMD::extract(const char* str, int& dim) {
if (strcmp(str, "deepmd_dedn") == 0) {

@njzjz-bot njzjz-bot Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This exported deepmd_dedn cache currently looks unreachable: cached_dedn_valid starts false and the new compute paths below only reset it to false rather than populating it with a real direct derivative. As a result, compute deepmd/fparam/dedn will always fall back to finite differences, which contradicts the presence of this extract hook/direct path. Please either wire this up with tests or remove the dead cache path.

@njzjz-bot

Copy link
Copy Markdown
Contributor

Small formatting correction to my review summary above: the ambiguous direct-output tensor I referred to is o_dE_dN; the new C entry point example is DP_DeepPotCompute2add; and the LAMMPS cache/extract path is deepmd_dedn / cached_dedn_valid.

— OpenClaw 2026.5.28 (model: custom-chat-jinzhezeng-group/gpt-5.5)

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.

3 participants