feat(pt/dpmodel): add use_default_pf#5356
feat(pt/dpmodel): add use_default_pf#5356iProzd wants to merge 3 commits intodeepmodeling:masterfrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| ) | ||
| pt_loss_val = pt_loss.detach().cpu().numpy() | ||
| # loss should be non-zero because pf loss is activated via use_default_pf | ||
| self.assertTrue(pt_loss_val != 0.0) |
Check notice
Code scanning / CodeQL
Imprecise assert Note test
|
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 (2)
📝 WalkthroughWalkthroughAdded a boolean flag Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deepmd/utils/argcheck.py`:
- Around line 3307-3313: The schema now accepts use_default_pf but the Paddle
training path silently ignores it; update the Paddle loss handling in
deepmd/pd/train/training.py so that when loss_params (the dict forwarded into
the loss constructor) contains "use_default_pf" it is either honored by passing
it into the Paddle loss implementation or rejected up-front. Specifically,
either (A) extend the Paddle loss constructor(s) referenced where loss_params is
passed (look for the loss creation block around the code forwarding loss_params
at lines ~1316-1329) to accept and act on use_default_pf, or (B) validate and
remove/raise on presence of "use_default_pf" before forwarding (e.g., check
loss_params.get("use_default_pf") and raise a clear error), and ensure the
change references the same loss_params variable and the Paddle training module
functions so behavior is consistent with the other backends.
In `@source/tests/pt/test_loss_default_pf.py`:
- Around line 198-206: The test binds unused variables pt_loss_without and
pt_loss_with from the loss_fn return tuple; change those bindings to dummy
underscores (e.g., replace pt_loss_without and pt_loss_with with _ or use a
single leading underscore name) or omit binding them entirely so the unpacked
but unused values don't trigger Ruff RUF059; update the two calls to loss_fn
(the ones assigning "_, pt_loss_without, pt_more_loss_without" and "_,
pt_loss_with, pt_more_loss_with") to use a dummy name for the middle element
(e.g., "_, _, pt_more_loss_without" and "_, _, pt_more_loss_with" or "_,
_pt_loss, pt_more_loss...") and run ruff format/check.
🪄 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: 860ed2bb-bc14-45f0-a96c-b7ec609a1629
📒 Files selected for processing (5)
deepmd/dpmodel/loss/ener.pydeepmd/pt/loss/ener.pydeepmd/utils/argcheck.pydoc/model/train-se-a-mask.mdsource/tests/pt/test_loss_default_pf.py
| Argument( | ||
| "use_default_pf", | ||
| bool, | ||
| optional=True, | ||
| default=False, | ||
| doc=doc_use_default_pf, | ||
| ), |
There was a problem hiding this comment.
use_default_pf is accepted here but can be a silent no-op on Paddle backend.
This option is now valid in schema, but the Paddle training path (deepmd/pd/train/training.py Line 1316-Line 1329) forwards loss_params into a loss constructor that does not consume use_default_pf as an active field, so users can set it without effect. Please either implement it in the Paddle loss path or explicitly reject it there to avoid backend-inconsistent behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deepmd/utils/argcheck.py` around lines 3307 - 3313, The schema now accepts
use_default_pf but the Paddle training path silently ignores it; update the
Paddle loss handling in deepmd/pd/train/training.py so that when loss_params
(the dict forwarded into the loss constructor) contains "use_default_pf" it is
either honored by passing it into the Paddle loss implementation or rejected
up-front. Specifically, either (A) extend the Paddle loss constructor(s)
referenced where loss_params is passed (look for the loss creation block around
the code forwarding loss_params at lines ~1316-1329) to accept and act on
use_default_pf, or (B) validate and remove/raise on presence of "use_default_pf"
before forwarding (e.g., check loss_params.get("use_default_pf") and raise a
clear error), and ensure the change references the same loss_params variable and
the Paddle training module functions so behavior is consistent with the other
backends.
| _, pt_loss_without, pt_more_loss_without = loss_fn( | ||
| {}, | ||
| fake_model, | ||
| self.label_without_pref, | ||
| self.nloc, | ||
| self.cur_lr, | ||
| ) | ||
| # With find_atom_pref=1.0, pf loss should be computed | ||
| _, pt_loss_with, pt_more_loss_with = loss_fn( |
There was a problem hiding this comment.
Fix Ruff RUF059: unused unpacked variables in tests.
pt_loss_without and pt_loss_with are assigned but unused. Rename to dummy variables (or avoid binding) so lint stays green.
🔧 Minimal fix
- _, pt_loss_without, pt_more_loss_without = loss_fn(
+ _, _pt_loss_without, pt_more_loss_without = loss_fn(
{},
fake_model,
self.label_without_pref,
self.nloc,
self.cur_lr,
)
@@
- _, pt_loss_with, pt_more_loss_with = loss_fn(
+ _, _pt_loss_with, pt_more_loss_with = loss_fn(
{},
fake_model,
self.label_with_pref,
self.nloc,
self.cur_lr,
)As per coding guidelines **/*.py: "Always run ruff check . and ruff format . before committing changes or CI will fail".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _, pt_loss_without, pt_more_loss_without = loss_fn( | |
| {}, | |
| fake_model, | |
| self.label_without_pref, | |
| self.nloc, | |
| self.cur_lr, | |
| ) | |
| # With find_atom_pref=1.0, pf loss should be computed | |
| _, pt_loss_with, pt_more_loss_with = loss_fn( | |
| _, _pt_loss_without, pt_more_loss_without = loss_fn( | |
| {}, | |
| fake_model, | |
| self.label_without_pref, | |
| self.nloc, | |
| self.cur_lr, | |
| ) | |
| # With find_atom_pref=1.0, pf loss should be computed | |
| _, _pt_loss_with, pt_more_loss_with = loss_fn( |
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 198-198: Unpacked variable pt_loss_without is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 206-206: Unpacked variable pt_loss_with is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/tests/pt/test_loss_default_pf.py` around lines 198 - 206, The test
binds unused variables pt_loss_without and pt_loss_with from the loss_fn return
tuple; change those bindings to dummy underscores (e.g., replace pt_loss_without
and pt_loss_with with _ or use a single leading underscore name) or omit binding
them entirely so the unpacked but unused values don't trigger Ruff RUF059;
update the two calls to loss_fn (the ones assigning "_, pt_loss_without,
pt_more_loss_without" and "_, pt_loss_with, pt_more_loss_with") to use a dummy
name for the middle element (e.g., "_, _, pt_more_loss_without" and "_, _,
pt_more_loss_with" or "_, _pt_loss, pt_more_loss...") and run ruff format/check.
There was a problem hiding this comment.
Pull request overview
Adds a use_default_pf option to energy loss implementations so the prefactor-force (pf) term can be used even when atom_pref.npy is absent by falling back to a default atom preference of 1.0.
Changes:
- Add
use_default_pfto PTEnergyStdLossand DPModelEnergyLoss, including serialization/deserialization updates. - Adjust label requirements so
forceis requested whenpfis used, and setatom_pref’s default to1.0. - Add PT unit tests for the new behavior and document the new config flag.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
deepmd/pt/loss/ener.py |
Implements use_default_pf behavior, updates label requirements, bumps serialization version. |
deepmd/dpmodel/loss/ener.py |
Implements use_default_pf behavior, updates atom_pref default, bumps serialization version. |
deepmd/utils/argcheck.py |
Exposes use_default_pf in loss/ener arg validation/docs. |
doc/model/train-se-a-mask.md |
Documents how to enable use_default_pf in training JSON. |
source/tests/pt/test_loss_default_pf.py |
Adds PT-only tests for use_default_pf, label requirements, and serialization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| find_atom_pref = ( | ||
| label_dict["find_atom_pref"] if not self.use_default_pf else 1.0 | ||
| ) |
There was a problem hiding this comment.
The use_default_pf behavior change in EnergyLoss.call() (overriding find_atom_pref) isn’t covered by the existing dpmodel loss unit tests (e.g., source/tests/common/dpmodel/test_loss_ener.py). Please add a small test case asserting that with find_atom_pref=0.0 and use_default_pf=True, the pf contribution/metrics are computed, and that with use_default_pf=False they remain suppressed.
| def get_single_batch(dataset, index=None): | ||
| if index is None: | ||
| index = dp_random.choice(np.arange(len(dataset))) | ||
| np_batch = dataset[index] |
There was a problem hiding this comment.
get_single_batch() picks a random frame index via dp_random.choice(...) but this test module never seeds dp_random, so the selected frame can vary between runs/environments. To avoid flaky tests, either pass an explicit index (e.g., 0) or seed dp_random in setUp() (consistent with other tests that call dp_random.seed(...)).
| } | ||
| ``` | ||
|
|
||
| If `atom_pref.npy` is not provided in the training data, one can set `use_default_pf` to `true` to use a default atom preference of 1.0 for all atoms. This allows using the prefactor force loss (`pf` loss) without requiring `atom_pref.npy` files. When `atom_pref.npy` is provided, it will be used as-is regardless of this setting. |
There was a problem hiding this comment.
The new paragraph implies use_default_pf works in general training, but the implementation appears to exist only in the PyTorch/DPModel losses (TF/Paddle EnerStdLoss accept **kwargs and will silently ignore use_default_pf). Please clarify the backend scope here (e.g., explicitly state “PyTorch/DP backend only”) or implement equivalent behavior for the other backends to avoid a configuration option that is accepted but has no effect.
| If `atom_pref.npy` is not provided in the training data, one can set `use_default_pf` to `true` to use a default atom preference of 1.0 for all atoms. This allows using the prefactor force loss (`pf` loss) without requiring `atom_pref.npy` files. When `atom_pref.npy` is provided, it will be used as-is regardless of this setting. | |
| If `atom_pref.npy` is not provided in the training data and you are using the PyTorch/DP backend, you can set `use_default_pf` to `true` to use a default atom preference of 1.0 for all atoms. This allows using the prefactor force loss (`pf` loss) without requiring `atom_pref.npy` files. When `atom_pref.npy` is provided, it will be used as-is regardless of this setting. For TensorFlow/Paddle backends (for example, when using `EnerStdLoss`), the `use_default_pf` option is accepted but ignored and therefore has no effect. |
| ) | ||
| doc_limit_pref_pf = limit_pref("atomic prefactor force") | ||
| doc_use_default_pf = ( | ||
| "If true, use default atom_pref of 1.0 for all atoms when atom_pref data is not provided. " | ||
| "This allows using the prefactor force loss (pf) without requiring atom_pref.npy files in training data. " | ||
| "When atom_pref.npy is provided, it will be used as-is regardless of this setting." |
There was a problem hiding this comment.
use_default_pf is now validated/documented for loss/ener, but TF/Paddle energy losses do not implement this behavior (they accept **kwargs and will ignore it). Consider documenting that this flag is only effective for PyTorch/DPModel (or add an explicit warning/error in non-supporting backends) to avoid a silent no-op configuration. Also, the existing start_pref_pf help text still states atom_pref.npy must be provided when prefactors are non-zero; with use_default_pf=true that statement is no longer universally true and may need to be adjusted.
| ) | |
| doc_limit_pref_pf = limit_pref("atomic prefactor force") | |
| doc_use_default_pf = ( | |
| "If true, use default atom_pref of 1.0 for all atoms when atom_pref data is not provided. " | |
| "This allows using the prefactor force loss (pf) without requiring atom_pref.npy files in training data. " | |
| "When atom_pref.npy is provided, it will be used as-is regardless of this setting." | |
| ) + ( | |
| " When `use_default_pf` is true, `atom_pref.npy` is not required even when the " | |
| "prefactor is non-zero; a default per-atom prefactor of 1.0 will be used when " | |
| "`atom_pref.npy` is missing. When `use_default_pf` is false, `atom_pref.npy` " | |
| "must still be provided whenever the prefactor is non-zero." | |
| ) | |
| doc_limit_pref_pf = limit_pref("atomic prefactor force") | |
| doc_use_default_pf = ( | |
| "If true, use default atom_pref of 1.0 for all atoms when atom_pref data is not provided. " | |
| "This allows using the prefactor force loss (pf) without requiring atom_pref.npy files in training data. " | |
| "When atom_pref.npy is provided, it will be used as-is regardless of this setting. " | |
| "Note: at present this option is only effective for the PyTorch/DPModel backend; " | |
| "other backends (e.g. TensorFlow and Paddle) accept this flag but ignore it." |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5356 +/- ##
==========================================
- Coverage 82.32% 82.32% -0.01%
==========================================
Files 809 810 +1
Lines 83161 83226 +65
Branches 4067 4066 -1
==========================================
+ Hits 68462 68514 +52
- Misses 13484 13497 +13
Partials 1215 1215 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
New Features
Behavior Changes
Documentation
Tests