improve: ase try to get virials from different sources #660
Conversation
📝 WalkthroughWalkthroughWalkthroughThe Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
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 (
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #660 +/- ##
=======================================
Coverage 85.38% 85.39%
=======================================
Files 82 82
Lines 7549 7552 +3
=======================================
+ Hits 6446 6449 +3
Misses 1103 1103 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #660 will not alter performanceComparing Summary
|
link89
force-pushed
the
support-ase-virial
branch
from
dd2ae5f to
451bd0d
Compare
![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 (2)
dpdata/plugins/ase.py (2)
109-109: Add validation for virial data format.Consider adding basic validation to ensure the virial data has the expected shape and format before using it, especially since it can come from user-provided sources.
virials = np.array([-atoms.get_volume() * stress]) + # Ensure virial has the expected shape (3, 3) + if virials.shape != (1, 3, 3): + virials = virials.reshape(1, 3, 3)
100-100: Remove unnecessary blank line.Minor whitespace cleanup to maintain consistent formatting.
- } - + } # try to get virials from different sources
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dpdata/plugins/ase.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
dpdata/plugins/ase.py (2)
dpdata/abacus/scf.py (1)
get_stress(227-232)dpdata/qe/scf.py (1)
get_stress(121-130)
🔇 Additional comments (1)
dpdata/plugins/ase.py (1)
65-68: Update documentation to include the missing 'virials' key.Based on past review comments, the implementation should support both 'virial' and 'virials' keys as a fallback strategy. However, the current documentation only mentions 'virial' from atoms.info, missing the 'virials' fallback.
- Note that this method will try to load virial from the following sources: - - atoms.info['virial'] - - converted from stress tensor + Note that this method will try to load virials from the following sources: + + - atoms.info['virial'] + - atoms.info['virials'] + - converted from stress tensorLikely an incorrect or invalid review comment.
|
Hi @njzjz Is there any chance to see this PR in the next release of dpdata? UNPROCESSED_KEYS = {'uid'}
SPECIAL_3_3_KEYS = {'Lattice', 'virial', 'stress'}
# partition ase.calculators.calculator.all_properties into two lists:
# 'per-atom' and 'per-config'ase does support reading |
|
Please let all checks pass. |
link89
force-pushed
the
support-ase-virial
branch
from
aa01708 to
d6906f6
Compare
|
Resolved. I think the code coverage is a false positive. |
2 participants
Summary by CodeRabbit