fix: add optional force check #780
Conversation
CodSpeed Performance ReportMerging #780 will not alter performanceComparing Summary
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## devel #780 +/- ##
==========================================
+ Coverage 85.17% 85.19% +0.01%
==========================================
Files 81 81
Lines 7534 7544 +10
==========================================
+ Hits 6417 6427 +10
Misses 1117 1117 ☔ View full report in Codecov by Sentry. |
📝 WalkthroughWalkthroughThe pull request introduces modifications to several components in the dpdata library, focusing on enhancing error handling and data processing. Key changes include the addition of conditional checks for the presence of optional data such as forces and virials across various file format parsers (ASE, PWmat, VASP). These adjustments aim to improve the robustness of the code by preventing potential runtime errors when certain data keys are missing or contain None values. Changes
Possibly related PRs
Suggested reviewers
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: 0
🧹 Nitpick comments (2)
dpdata/ase_calculator.py (1)
65-66: Consider handling requested properties explicitly.While the force check is good, the method ignores the
propertiesparameter. If "forces" is in the requested properties but not in data, this should potentially raise aPropertyNotImplementedErrorto align with ASE's conventions.if "forces" in data: self.results["forces"] = data["forces"][0] +elif "forces" in properties: + raise PropertyNotImplementedError("Forces not available in data")dpdata/plugins/ase.py (1)
178-180: Consider pre-building results dictionary.While the conditional checks are good, building the dictionary incrementally might be less efficient than pre-building it with known values.
-results = {"energy": data["energies"][ii]} -if "forces" in data: - results["forces"] = data["forces"][ii] +results = { + "energy": data["energies"][ii], + **({'forces': data["forces"][ii]} if "forces" in data else {}) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
dpdata/ase_calculator.py(1 hunks)dpdata/plugins/ase.py(1 hunks)dpdata/plugins/pwmat.py(1 hunks)dpdata/plugins/vasp.py(2 hunks)
🔇 Additional comments (2)
dpdata/plugins/pwmat.py (1)
34-40: LGTM! Robust handling of optional force data.The changes correctly handle cases where forces might be None, preventing potential runtime errors.
dpdata/plugins/vasp.py (1)
Line range hint
84-94: LGTM! Consistent implementation across parsers.The changes maintain consistency with other parsers while adding robustness against None values.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dpdata/driver.py (1)
169-170: LGTM! Consider consolidating the dictionary key checks.The added safety check prevents KeyError when forces are missing. For better readability, consider using the same pattern for both forces and virials:
- if "forces" in labeled_data and "forces" in lb_data: - labeled_data["forces"] += lb_data["forces"] - if "virials" in labeled_data and "virials" in lb_data: - labeled_data["virials"] += lb_data["virials"] + for key in ["forces", "virials"]: + if key in labeled_data and key in lb_data: + labeled_data[key] += lb_data[key]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dpdata/driver.py(1 hunks)dpdata/plugins/ase.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
dpdata/plugins/ase.py
301-301: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
301-301: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
313-313: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
🔇 Additional comments (2)
dpdata/plugins/ase.py (2)
178-180: LGTM! Forces are now conditionally included.The change correctly makes forces optional in the results dictionary, consistent with the PR objective.
178-180: Verify handling of optional forces across the codebase.The changes make forces optional, which is good. However, we should verify that other parts of the codebase don't assume forces are always present.
Also applies to: 301-304
✅ Verification successful
Optional forces handling is consistent with codebase patterns
The codebase already handles forces as an optional attribute across various plugins and core implementations. The changes in ase.py align with these existing patterns. No instances found where forces are assumed to always exist in the implementation files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct dictionary access of 'forces' key that might assume its existence rg "(\[|\.get)\s*['\"]forces['\"]" -t py # Search for error handling patterns around forces rg "try.*forces.*except" -t pyLength of output: 5788
Summary by CodeRabbit
Bug Fixes
Refactor