Feat: support fparam/aparam in dp calculator #4819
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ASE_Atoms
participant DP_Calculator
participant DP_Model
User->>ASE_Atoms: Set up Atoms (with fparam, aparam in info)
User->>DP_Calculator: Request calculation (energy, forces, stress)
DP_Calculator->>ASE_Atoms: Access info['fparam'], info['aparam']
DP_Calculator->>DP_Model: eval(coords, cells, atom_types, fparam, aparam)
DP_Model-->>DP_Calculator: Return energy, forces, virial, etc.
DP_Calculator-->>User: Return results
sequenceDiagram
participant TestCase
participant DP_Calculator
participant ASE_Atoms
TestCase->>DP_Calculator: Initialize with model (with fparam/aparam)
TestCase->>ASE_Atoms: Create Atoms (with fparam, aparam)
TestCase->>DP_Calculator: Calculate properties
TestCase->>ASE_Atoms: Permute atoms and parameters
TestCase->>DP_Calculator: Recalculate properties
TestCase->>TestCase: Assert invariance and correct shapes
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (3.3.7)deepmd/calculator.pyNo files to lint: exiting. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (29)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
deepmd/calculator.py (1)
135-137: Good implementation of parameter passing.The modification correctly passes the optional
fparamandaparamparameters to the model evaluation. Using keyword arguments and None defaults is appropriate.Consider updating the method docstring to document the new
fparamandaparamsupport for better API documentation:def calculate( self, atoms: Optional["Atoms"] = None, properties: list[str] = ["energy", "forces", "virial"], system_changes: list[str] = all_changes, ) -> None: """Run calculation with deepmd model. Parameters ---------- atoms : Optional[Atoms], optional atoms object to run the calculation on, by default None + atoms.info may contain 'fparam' and 'aparam' for additional model parameters properties : list[str], optional unused, only for function signature compatibility, by default ["energy", "forces", "stress"] system_changes : list[str], optional unused, only for function signature compatibility, by default all_changes """source/tests/pt/test_calculator.py (1)
144-145: Verify parameter data types for testing.The test uses
IntTensorforfparamandaparam, but these typically represent physical parameters which are usually floating-point values. This might be intentional for testing to avoid floating-point precision issues.Consider using floating-point tensors for more realistic testing:
- fparam = torch.IntTensor([1, 2]) - aparam = torch.IntTensor([[1], [0], [2], [1], [0]]) + fparam = torch.tensor([1.0, 2.0], dtype=dtype) + aparam = torch.tensor([[1.0], [0.0], [2.0], [1.0], [0.0]], dtype=dtype)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deepmd/calculator.py(1 hunks)source/tests/pt/test_calculator.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (29)
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Test Python (3, 3.9)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Test Python (6, 3.12)
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Test Python (5, 3.12)
- GitHub Check: Test Python (5, 3.9)
- GitHub Check: Test Python (4, 3.12)
- GitHub Check: Test Python (6, 3.9)
- GitHub Check: Test Python (4, 3.9)
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Build C library (2.14, >=2.5.0rc0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Test C++ (false)
- GitHub Check: Test C++ (true)
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Build C++ (cuda, cuda)
🔇 Additional comments (3)
source/tests/pt/test_calculator.py (3)
113-114: Good test configuration for fparam/aparam support.The model configuration correctly sets up fitting and atomic parameters for testing the new functionality.
160-160: Excellent handling of parameter assignment and permutation.The test correctly assigns parameters to
atoms.infoand properly handles the fact thatfparamis system-wide whileaparamis per-atom (correctly permuted withaparam[idx_perm, :]).Also applies to: 175-175
182-188: Comprehensive test assertions validate the implementation.The test properly verifies data types, shapes, and most importantly, the invariance of computed properties under atom permutation. This ensures the fparam/aparam functionality doesn't break physical symmetries.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## devel #4819 +/- ##
=======================================
Coverage 84.57% 84.57%
=======================================
Files 699 699
Lines 68035 68036 +1
Branches 3540 3540
=======================================
+ Hits 57539 57544 +5
+ Misses 9361 9358 -3
+ Partials 1135 1134 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced support for optional fitting and atomic parameters during calculations. - **Tests** - Added tests to verify correct handling of models with multiple fitting and atomic parameters, ensuring consistent results under atom permutation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Summary by CodeRabbit