improves the robustness of VASP OUTCAR energy extraction#24
improves the robustness of VASP OUTCAR energy extraction#24floatingCatty merged 4 commits intodeepmodeling:mainfrom
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
📝 WalkthroughWalkthroughThe VASP parser's energy reading method is enhanced to detect incomplete OUTCAR runs by checking for a "Voluntary context switches" completion indicator. A warning is issued when energy is extracted from files lacking this indicator. New tests verify the warning is issued or suppressed based on run completion status. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![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 (3)
dftio/io/vasp/vasp_parser.py (1)
110-112: Consider memory efficiency for large OUTCAR files.Reading the entire OUTCAR file into memory with
readlines()may be inefficient for very large files. Consider iterating line-by-line instead:- with open(file, 'r') as f: - data = f.readlines() - for line in data: + with open(file, 'r') as f: + for line in f:This approach processes the file line-by-line without loading it entirely into memory.
test/test_vasp_energy.py (2)
216-216: Remove unnecessary f-string prefix.The f-string has no placeholders, making the prefix unnecessary.
Apply this diff:
- print(f"Unsuccessful completion warning test passed") + print("Unsuccessful completion warning test passed")
253-253: Remove unnecessary f-string prefix.The f-string has no placeholders, making the prefix unnecessary.
Apply this diff:
- print(f"Successful completion (no warning) test passed") + print("Successful completion (no warning) test passed")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dftio/io/vasp/vasp_parser.py(1 hunks)test/test_vasp_energy.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/test_vasp_energy.py (1)
dftio/io/vasp/vasp_parser.py (2)
VASPParser(18-125)read_total_energy(94-125)
🪛 Ruff (0.14.5)
test/test_vasp_energy.py
216-216: f-string without any placeholders
Remove extraneous f prefix
(F541)
253-253: f-string without any placeholders
Remove extraneous f prefix
(F541)
🔇 Additional comments (6)
dftio/io/vasp/vasp_parser.py (3)
94-94: LGTM: Good addition of default parameter and return type annotation.The default parameter and type hint improve the method's usability and clarity.
95-107: LGTM: Comprehensive docstring with appropriate warnings.The expanded docstring clearly documents the method's behavior and warns users about potential reliability issues with unsuccessful runs.
121-122: LGTM: Clear warning message for unsuccessful completions.The warning appropriately alerts users when energy is extracted from potentially incomplete runs, including the filename for context.
test/test_vasp_energy.py (3)
184-218: LGTM: Comprehensive test for unsuccessful completion detection.The test properly verifies that:
- Energy extraction still functions for incomplete runs
- A warning is logged when the completion indicator is absent
The test structure with log capture is well-implemented.
221-255: LGTM: Thorough test for successful completion (no warning case).The test properly verifies the absence of warnings when the completion indicator is present, complementing the unsuccessful completion test nicely.
265-266: LGTM: New tests properly registered.Both new test functions are correctly added to the test execution block.
| success_completion = False | ||
| energy = [] | ||
| with open(file, 'r') as f: | ||
| data = f.readlines() | ||
| for line in data: | ||
| if 'Voluntary context switches' in line: | ||
| success_completion = True |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the reliability of "Voluntary context switches" as a completion indicator.
The implementation uses "Voluntary context switches" as the indicator for successful VASP completion. Please confirm this is a reliable and documented indicator in VASP OUTCAR files.
🌐 Web query:
Is "Voluntary context switches" a reliable indicator of successful completion in VASP OUTCAR files?
💡 Result:
Short answer: It's a useful but not foolproof heuristic — the presence of the final "Voluntary context switches" (the last line of the VASP timing/accounting block) usually means VASP reached its normal end and printed the final accounting, and many scripts rely on it to mark a finished run. However, it is not guaranteed: some abnormal terminations may omit the accounting block (so the line will be absent even for failed jobs), and other checks (e.g., explicit convergence messages in OUTCAR/OSZICAR, existence/contents of CONTCAR/CHGCAR/OSZICAR, "General timing and accounting"/Total CPU time used) are recommended for robust detection. See examples in storq and common VASP utilities that use this line but also note they treat it as a heuristic. [1][2]
Sources:
- storq reader: read_outcar_validity uses "Voluntary context switches" to mark proper finish and notes cancelled/error runs may not print it. [1]
- ASE/Jasp and community examples showing many tools/scripts check the OUTCAR tail for "Voluntary context switches" as a finished-job indicator (but other checks are commonly used). [2]
Add more robust completion detection beyond "Voluntary context switches" line.
The "Voluntary context switches" line is a commonly-used heuristic in VASP tools (storq, ASE/Jasp), but it's not reliable for all cases. Abnormal terminations may omit the accounting block entirely, and some failed jobs may still contain this line.
Improve robustness by checking additional signals: explicit convergence messages in OUTCAR/OSZICAR, existence/contents of CONTCAR/CHGCAR/OSZICAR, or "General timing and accounting"/Total CPU time. At minimum, add a comment noting this is a heuristic and document the limitations.
🤖 Prompt for AI Agents
In dftio/io/vasp/vasp_parser.py around lines 108 to 114, the current completion
detection relies solely on the "Voluntary context switches" line which is a
heuristic and can be false-positive/negative; update the logic to be more robust
by: keep the existing check but also scan the OUTCAR/OSZICAR contents for
explicit convergence/termination phrases (e.g., "reached required accuracy",
"reached required accuracy - stopping structural energy minimisation", "General
timing and accounting", "Total CPU time"), verify existence and non-empty status
of key output files (CONTCAR, OSZICAR, CHGCAR) as additional success signals,
and treat completion as true only if one or more of these indicators are
present; if full checks are not implemented, add a clear inline comment
describing this heuristic and its limitations.
2 participants
This pull request improves the robustness of VASP OUTCAR energy extraction by adding a check for successful run completion and enhancing test coverage. Now, when extracting the total energy from an OUTCAR file, the parser logs a warning if the file does not indicate successful completion, helping users identify potentially unreliable results. The test suite is expanded to verify this new behavior.
VASP OUTCAR energy extraction improvements:
read_total_energyinvasp_parser.pyto check for the presence of the "Voluntary context switches" line, logging a warning if it is missing (indicating an unsuccessful run), while still extracting the energy if possible.Test enhancements:
test_vasp_unsuccessful_completion_warningto verify that a warning is logged when OUTCAR does not indicate successful completion, and that energy extraction still works.test_vasp_successful_completion_no_warningto confirm that no warning is logged when OUTCAR indicates a successful run.Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.