fix(lmp): default fparam support#5311
Merged
iProzd merged 5 commits intodeepmodeling:masterfrom Mar 15, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds end-to-end support for default frame parameters (“default fparam”) across the C++/C APIs and backends (notably PyTorch/JAX), enabling callers (e.g., LAMMPS paths) to omit fparam and have the model’s default applied.
Changes:
- Add
has_default_fparam()plumbing to the C++ backend interface and expose it through the C anddeepmd.hppwrapper APIs. - Update
deepmd.hppvalidation to allow an emptyfparamwhen the model reports a default is available. - Add new inference fixtures/tests (C and C++/PyTorch) to validate empty-
fparambehavior and backward compatibility with explicitfparam.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| source/tests/infer/fparam_aparam_default.yaml | Adds a model serialization fixture containing default_fparam. |
| source/api_cc/tests/test_deeppot_default_fparam_pt.cc | New gtest coverage for PyTorch backend using empty vs explicit fparam. |
| source/api_cc/src/DeepSpinPT.cc | Reads has_default_fparam from TorchScript during init. |
| source/api_cc/src/DeepPotPT.cc | Reads has_default_fparam from TorchScript during init. |
| source/api_cc/src/DeepPotJAX.cc | Reads has_default_fparam from exported JAX/TF functions when present. |
| source/api_cc/src/DeepBaseModel.cc | Exposes DeepBaseModel::has_default_fparam() passthrough. |
| source/api_cc/include/DeepSpinTF.h | Defines TF backend behavior for has_default_fparam() (always false). |
| source/api_cc/include/DeepSpinPT.h | Adds has_default_fparam() API + member storage for PT backend. |
| source/api_cc/include/DeepPotTF.h | Defines TF backend behavior for has_default_fparam() (always false). |
| source/api_cc/include/DeepPotPT.h | Adds has_default_fparam() API + member storage for PT backend. |
| source/api_cc/include/DeepPotPD.h | Defines Paddle backend behavior for has_default_fparam() (always false). |
| source/api_cc/include/DeepPotJAX.h | Adds has_default_fparam() API + member storage for JAX backend. |
| source/api_cc/include/DeepBaseModel.h | Extends backend interface with new pure virtual has_default_fparam(). |
| source/api_c/tests/test_deeppot_default_fparam.cc | New C-API wrapper test validating empty fparam uses defaults. |
| source/api_c/src/c_api.cc | C API struct caching now includes has_default_fparam and exposes query functions. |
| source/api_c/include/deepmd.hpp | Adds has_default_fparam() to wrapper and allows empty fparam when supported. |
| source/api_c/include/c_api_internal.h | Extends C-API internal structs with has_default_fparam. |
| source/api_c/include/c_api.h | Adds new C API query functions for default-fparam support. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a has_default_fparam flag and matching accessors across C and C++ APIs and backend implementations, bumps C API version to 26, and exposes new C-accessor functions. Includes unit tests, LAMMPS Python tests, and a YAML model fixture to validate default vs explicit fparam behavior. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
![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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/api_cc/include/DeepPotTF.h`:
- Around line 213-217: The method has_default_fparam() should not
unconditionally return false; instead add a private bool member (e.g.,
has_default_fparam_cached_) to class DeepPotTF, set that flag during init() when
parsing the SavedModel (use whatever logic already inspects get_default_fparam()
/ serialized metadata in init()), and make has_default_fparam() return the
cached value; ensure init() updates the cache even when called multiple times
and default to false until init() runs.
In `@source/api_cc/include/DeepSpinTF.h`:
- Around line 179-183: The has_default_fparam() method in class DeepSpinTF
should not unconditionally return false; instead return the parsed model state
indicating whether default frame parameters were serialized. Update
DeepSpinTF::has_default_fparam() to consult the TF backend's parsed
metadata/member (e.g., the parsed flag or stored default_fparam_ state populated
when loading the SavedModel) and return that boolean; ensure consistency with
DeepSpinTF::get_default_fparam() so that if the model contained default fparam
v3.1.2+ data the API reports true and get_default_fparam() will be valid.
In `@source/api_cc/src/DeepPotJAX.cc`:
- Around line 330-335: The catch that sets has_default_fparam_ = false when
get_scalar<bool>(ctx, "has_default_fparam", func_vector, device, status) throws
misreports older exports; instead, on catching tf_function_not_found, attempt to
recover capability from persisted SavedModel metadata: call
get_scalar<string>(ctx, "jax_exporter_version", func_vector, device, status) (or
the equivalent exporter/version key used in your exports), parse the semver and
if the exporter version is older than 3.1.2 then read the persisted default
fparam value via get_scalar<bool>(ctx, "get_default_fparam" or your stored
default fparam key, using the same ctx/func_vector/device/status) and set
has_default_fparam_ accordingly (if that key is absent, choose a conservative
default and/or surface a clear compatibility warning); only default to false if
metadata proves the model lacks default fparam or no recovery is possible and
document this compatibility hole.
In `@source/api_cc/src/DeepPotPT.cc`:
- Around line 149-152: The try/catch currently using catch(...) around
module.run_method("has_default_fparam") hides real runtime/type errors; instead
check for method existence with module.find_method("has_default_fparam") (or
equivalent API) and only call module.run_method("has_default_fparam") if it
exists, setting has_default_fparam_ accordingly; if you must call and catch,
catch the specific missing-method exception type (matching the translate_error
pattern used elsewhere) rather than using catch(...), so real errors from the
scripted method are not silently converted to false.
In `@source/api_cc/src/DeepSpinPT.cc`:
- Around line 121-125: The catch-all around
module.run_method("has_default_fparam") masks real errors; change it to handle
only the method-not-found case (as in DeepPotJAX.cc) or rethrow via
translate_error() so real exceptions propagate. Locate the block that sets
has_default_fparam_ (module.run_method("has_default_fparam") and
has_default_fparam_) and replace the bare catch(...) with either a specific
exception type check for a missing method or call translate_error() inside the
catch to surface real errors while still treating a true "method absent"
condition as false.
In `@source/api_cc/tests/test_deeppot_default_fparam_pt.cc`:
- Around line 23-26: The coord vector initializer contains a likely-typo numeric
literal "00.25" inside the std::vector<VALUETYPE> coord initializer; update that
element to "0.25" in the coord initialization (the std::vector<VALUETYPE> coord
initializer in test_deeppot_default_fparam_pt.cc) so the value is written in
conventional numeric form.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: db69dfcd-284a-49ee-b67e-8b1073a4cf76
📒 Files selected for processing (19)
source/api_c/include/c_api.hsource/api_c/include/c_api_internal.hsource/api_c/include/deepmd.hppsource/api_c/src/c_api.ccsource/api_c/tests/test_deeppot_default_fparam.ccsource/api_cc/include/DeepBaseModel.hsource/api_cc/include/DeepPotJAX.hsource/api_cc/include/DeepPotPD.hsource/api_cc/include/DeepPotPT.hsource/api_cc/include/DeepPotTF.hsource/api_cc/include/DeepSpinPT.hsource/api_cc/include/DeepSpinTF.hsource/api_cc/src/DeepBaseModel.ccsource/api_cc/src/DeepPotJAX.ccsource/api_cc/src/DeepPotPT.ccsource/api_cc/src/DeepSpinPT.ccsource/api_cc/tests/test_deeppot_default_fparam_pt.ccsource/tests/infer/fparam_aparam_default.pthsource/tests/infer/fparam_aparam_default.yaml
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #5311 +/- ##
==========================================
- Coverage 82.30% 82.30% -0.01%
==========================================
Files 773 775 +2
Lines 77414 77627 +213
Branches 3659 3677 +18
==========================================
+ Hits 63715 63888 +173
- Misses 12528 12566 +38
- Partials 1171 1173 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
njzjz
requested changes
Mar 12, 2026
njzjz
approved these changes
Mar 14, 2026
wanghan-iapcm
approved these changes
Mar 14, 2026
Collaborator
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
For me it looks ok, but please carefully resolve the comment from the AI reviewers. They look reasonable.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
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