Feat: set force label optional #772
Conversation
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughThe changes made in the Changes
Sequence Diagram(s)Error: Could not generate a valid Mermaid diagram after multiple attempts. 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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## devel #772 +/- ##
=======================================
Coverage 85.16% 85.17%
=======================================
Files 81 81
Lines 7530 7534 +4
=======================================
+ Hits 6413 6417 +4
Misses 1117 1117 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #772 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
https://github.com/anyangml/dpdata/blob/9c3c81e537f1676d8b8e6529cf983095cdb417ac/dpdata/system.py#L1319
please not here we need to check the existence of the force label.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dpdata/system.py (1)
1319-1320: Add a comment explaining optional forces handling.The forces correction logic is correct, but it would be helpful to add a comment explaining why forces might be absent in either system.
- if "forces" in self.data and "forces" in hl_sys.data: + # Forces may be absent in either system since they are optional + if "forces" in self.data and "forces" in hl_sys.data:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dpdata/system.py(3 hunks)
🔇 Additional comments (3)
dpdata/system.py (3)
1212-1216: LGTM! Forces data type is now optional.The change to make forces optional is well-implemented and consistent with other optional data types in the class.
1276-1278: LGTM! Well-implemented forces presence check.The
has_forcesmethod follows the same pattern ashas_virialand provides a clean way to check for the presence of forces data.
1285-1286: LGTM! Safe handling of optional forces in affine transformation.The method now safely handles the case when forces data is not present, preventing potential errors.
2 participants
Summary by CodeRabbit