fix: add numb_fparam & numb_aparam to dipole & polar fitting
#4405
Conversation
Fix deepmodeling#4396. Fix deepmodeling#4397. Fix deepmodeling#4398. Throw errors for TF. Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
source/tests/consistent/model/test_dipole.py (1)
65-65: Consider adding more test cases for better coverage.The test suite would benefit from additional test cases to validate:
- Error handling for invalid
numb_fparamvalues (> 0)- Different valid
numb_fparamvalues- The new
numb_aparamparameter mentioned in PR objectivesHere's a suggested approach to add these test cases:
def test_invalid_numb_fparam(self): """Test that ValueError is raised when numb_fparam > 0.""" invalid_data = self.data.copy() invalid_data["fitting_net"]["numb_fparam"] = 1 with self.assertRaises(ValueError): self.pass_data_to_cls(self.dp_class, invalid_data) def test_numb_aparam(self): """Test configuration with numb_aparam parameter.""" data_with_aparam = self.data.copy() data_with_aparam["fitting_net"]["numb_aparam"] = 0 model = self.pass_data_to_cls(self.dp_class, data_with_aparam) self.assertEqual(model.get_numb_aparam(), 0)doc/model/train-fitting-tensor.md (1)
250-250: Enhance the documentation of TensorFlow backend limitationsWhile the note about TensorFlow's limitations is accurate, consider enhancing it to be more helpful to users:
- Make it more prominent by moving it to a warning/caution block
- Add context about why these parameters aren't supported
- Provide guidance on alternatives if available
Consider updating the note like this:
-The TensorFlow backend does not support {ref}`numb_fparam <model[standard]/fitting_net[dipole]/numb_fparam>` and {ref}`numb_aparam <model[standard]/fitting_net[dipole]/numb_aparam>`. +:::{warning} +The TensorFlow backend does not support {ref}`numb_fparam <model[standard]/fitting_net[dipole]/numb_fparam>` and {ref}`numb_aparam <model[standard]/fitting_net[dipole]/numb_aparam>`. Attempting to use these parameters with TensorFlow will raise a ValueError. If you need these parameters, consider using PyTorch or other supported backends. +:::deepmd/tf/fit/dipole.py (1)
439-463: LGTM with a minor docstring enhancement suggestionThe implementation of the
input_requirementproperty and parameter getters is correct. Consider enhancing the docstrings for the getter methods to include return type hints:def get_numb_fparam(self) -> int: - """Get the number of frame parameters.""" + """Get the number of frame parameters. + + Returns + ------- + int + The number of frame parameters + """ return self.numb_fparam def get_numb_aparam(self) -> int: - """Get the number of atomic parameters.""" + """Get the number of atomic parameters. + + Returns + ------- + int + The number of atomic parameters + """ return self.numb_aparamdeepmd/tf/fit/polar.py (2)
163-174: Consider improving error messages for parameter validation.While the validation logic is correct, the error messages could be more descriptive to help users understand why these parameters are not supported.
Consider updating the error messages to be more specific:
- raise ValueError("numb_fparam is not supported in the dipole fitting") + raise ValueError("Frame parameters (numb_fparam) are not supported in polar fitting as they may affect the rotational invariance of the polarizability tensor") - raise ValueError("numb_aparam is not supported in the dipole fitting") + raise ValueError("Atomic parameters (numb_aparam) are not supported in polar fitting as they may affect the rotational invariance of the polarizability tensor")
822-828: Consider adding validation in getter methods.The getter methods
get_numb_fparamandget_numb_aparamare implemented correctly, but they might benefit from additional validation to maintain consistency with the constructor's validation.Consider updating the methods to include validation:
def get_numb_fparam(self) -> int: """Get the number of frame parameters.""" + if self.numb_fparam > 0: + raise ValueError("Frame parameters are not supported in polar fitting") return self.numb_fparam def get_numb_aparam(self) -> int: """Get the number of atomic parameters.""" + if self.numb_aparam > 0: + raise ValueError("Atomic parameters are not supported in polar fitting") return self.numb_aparam
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
deepmd/tf/fit/dipole.py(6 hunks)deepmd/tf/fit/polar.py(6 hunks)deepmd/utils/argcheck.py(3 hunks)doc/model/train-fitting-tensor.md(1 hunks)source/tests/consistent/model/test_dipole.py(1 hunks)source/tests/consistent/model/test_polar.py(1 hunks)source/tests/consistent/model/test_property.py(1 hunks)
🔇 Additional comments (13)
source/tests/consistent/model/test_property.py (1)
60-60: LGTM! Verify consistency across test files.
The addition of numb_fparam: 0 to the fitting_net configuration aligns with the PR objectives and matches the validation requirements.
Let's verify if similar changes are applied consistently in the related test files:
✅ Verification successful
Changes are consistently applied across test files
The verification shows that numb_fparam: 0 is consistently added in all relevant test files:
- test_property.py
- test_polar.py
- test_dos.py
- test_dipole.py
The implementation is uniform across the test suite, which aligns with the PR objectives to add numb_fparam support.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if numb_fparam is consistently added across test files
# Expected: Should find similar changes in test_dipole.py and test_polar.py
# Search for numb_fparam in test files
rg -A 5 "numb_fparam" "source/tests/consistent/model/test_*.py"
# Search for fitting_net dictionary definitions
ast-grep --pattern 'fitting_net = {$$$}' "source/tests/consistent/model/test_*.py"
ast-grep --pattern '"fitting_net": {$$$}' "source/tests/consistent/model/test_*.py"
Length of output: 488
Script:
#!/bin/bash
# Let's first find the test files and then search within them
# Find test files in the model directory
fd "test_.*\.py$" source/tests/consistent/model/
# Search for numb_fparam in these files
rg "numb_fparam" source/tests/consistent/model/
# Search for fitting_net dictionary definitions
rg -A 5 "fitting_net.*=.*{" source/tests/consistent/model/
rg -A 5 "\"fitting_net\".*:.*{" source/tests/consistent/model/
Length of output: 1092
source/tests/consistent/model/test_polar.py (1)
65-65: LGTM! Consider adding test cases for non-zero numb_fparam.
The addition of numb_fparam to the test data aligns with the PR objectives. However, the current test only covers the case where numb_fparam is 0.
Let's verify if there are any test cases for non-zero values:
source/tests/consistent/model/test_dipole.py (1)
65-65: LGTM! The addition of numb_fparam aligns with PR objectives.
The change correctly adds the numb_fparam parameter to the dipole fitting configuration with a default value of 0.
deepmd/tf/fit/dipole.py (4)
32-34: LGTM: Import addition is appropriate
The addition of DataRequirementItem import is necessary for implementing the new input_requirement property.
57-60: LGTM: Constructor parameters properly documented and added
The numb_fparam and numb_aparam parameters are well-documented in the class docstring and appropriately added to the constructor with default values.
Also applies to: 85-86
396-397: LGTM: Serialization properly updated
The numb_fparam and numb_aparam parameters are correctly included in the serialized output.
122-125: Verify: Parameter validation seems to contradict PR objectives
The PR aims to add numb_fparam & numb_aparam support, but the implementation raises ValueError if these parameters are greater than 0. This seems to contradict the PR objectives. Please verify if this is intentional or if the validation should be removed.
deepmd/tf/fit/polar.py (4)
62-65: LGTM: Parameter documentation is clear and consistent.
The new parameters numb_fparam and numb_aparam are well-documented in the class docstring and properly added to the constructor with appropriate default values.
Also applies to: 96-97
589-590: LGTM: Proper serialization of new parameters.
The new parameters are correctly included in the serialization method, maintaining backward compatibility.
37-39: LGTM: Clean import of DataRequirementItem.
The import of DataRequirementItem is properly placed and follows the project's import style.
804-820: Verify the input requirements implementation.
The input_requirement property correctly constructs the data requirements based on numb_fparam and numb_aparam. However, since these parameters are not supported (as per the validation in the constructor), this property will always return an empty list.
deepmd/utils/argcheck.py (2)
Line range hint 1598-1627: LGTM: Parameters added correctly to polar fitting
The addition of numb_fparam and numb_aparam parameters to the polar fitting configuration is implemented correctly with proper documentation and default values.
1668-1690: LGTM: Parameters added consistently to dipole fitting
The addition of numb_fparam and numb_aparam parameters to the dipole fitting configuration matches the implementation in polar fitting, maintaining consistency across the codebase.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #4405 +/- ##
==========================================
- Coverage 84.64% 84.63% -0.02%
==========================================
Files 614 614
Lines 57007 57061 +54
Branches 3486 3487 +1
==========================================
+ Hits 48254 48293 +39
- Misses 7627 7644 +17
+ Partials 1126 1124 -2 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Fix #4396. Fix #4397. Fix #4398. Throw errors for TF.
Summary by CodeRabbit
Release Notes
New Features
numb_fparamandnumb_aparamfor improved fitting configurations in both dipole and polar fitting classes.Documentation
Bug Fixes