Feat: set force label optional #772
Conversation
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughThe pull request introduces a modification to the Changes
Sequence DiagramclassDiagram
class LabeledSystem {
+DataType forces (optional)
+create_instance()
}
note over LabeledSystem: Forces data can now be omitted
The sequence diagram illustrates the updated 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)
Finishing Touches
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
🧹 Nitpick comments (1)
dpdata/system.py (1)
1211-1215: Update class docstring to reflect optional forces.The
LabeledSystemclass docstring should be updated to indicate that forces are optional. Consider updating this section:For example, a labeled water system named `d_example` has two molecules (6 atoms) and `nframes` frames. The labels can be accessed by - `d_example['energies']` : a numpy array of size nframes - - `d_example['forces']` : a numpy array of size nframes x 6 x 3 + - `d_example['forces']` : optional, a numpy array of size nframes x 6 x 3 - `d_example['virials']` : optional, a numpy array of size nframes x 3 x 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dpdata/system.py(1 hunks)
🔇 Additional comments (2)
dpdata/system.py (2)
1211-1215: LGTM! Making forces optional is a reasonable change.This change allows creating labeled systems without force data, which is useful for quantum chemistry calculations that only output energies. The implementation is consistent with how other optional data types like
virialsare handled in the codebase.
1211-1215: Verify all usages of forces data in the codebase.Let's check for any code that might assume forces data is always present.
✅ Verification successful
Making forces optional is safe and consistent with codebase patterns
The codebase already handles cases where forces are not present, with multiple format parsers and tests explicitly supporting this scenario. No code was found that assumes forces must exist.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of forces data that might need updating # Find direct accesses to forces data echo "=== Direct accesses to forces data ===" rg "[\"\']forces[\"\']" -A 3 # Find methods that might use forces echo -e "\n=== Methods potentially using forces ===" ast-grep --pattern 'def $FUNC($$$) { $$$ forces $$$ }'Length of output: 366
Script:
#!/bin/bash # Search for force-related patterns with context echo "=== Force/Forces string literals ===" rg "forces|force" -g "*.py" -A 2 echo -e "\n=== Force-related variable assignments ===" ast-grep --pattern 'forces = $_' echo -e "\n=== Force-related method parameters ===" ast-grep --pattern 'def $FUNC($$$) { $$$ force $$$ }' echo -e "\n=== Force-related assertions or requirements ===" rg "assert.*force|require.*force" -g "*.py"Length of output: 70062
| "forces", | ||
| np.ndarray, | ||
| (Axis.NFRAMES, Axis.NATOMS, 3), | ||
| required=False, | ||
| deepmd_name="force", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add defensive checks for optional forces data.
Since forces are now optional, methods that use forces data should be updated to handle the case when forces are not present. Consider adding checks in the following methods:
affine_map_fvrot_frame_lower_triangularcorrection
Example pattern:
def affine_map_fv(self, trans, f_idx):
assert np.linalg.det(trans) != 0
if 'forces' in self.data:
self.data['forces'][f_idx] = np.matmul(self.data['forces'][f_idx], trans)
if self.has_virial():
self.data['virials'][f_idx] = np.matmul(trans.T, np.matmul(self.data['virials'][f_idx], trans))
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## devel #772 +/- ##
==========================================
- Coverage 85.16% 85.02% -0.15%
==========================================
Files 81 81
Lines 7528 7517 -11
==========================================
- Hits 6411 6391 -20
- Misses 1117 1126 +9 ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #772 will not alter performanceComparing Summary
|
Summary by CodeRabbit