-
Notifications
You must be signed in to change notification settings - Fork 850
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
read mag
from OSZICAR
#3146
read mag
from OSZICAR
#3146
Conversation
@chiang-yuan Yes, we definitely need a test for this. Do I understand correctly that your PR adds a |
Haven't tested different OSZICAR yet. Looks like OSZICAR test in test_ouptut.py doesn't test different combinations of available field. I am going to add a test considering if the field is missing. pymatgen/pymatgen/io/vasp/tests/test_outputs.py Lines 1422 to 1428 in 1b04a0a
|
Hi @janosh I think this PR is ready to be merged. Let me know if I can do anything to make it better! |
r"SP=\s*([\d\-\.E\+]+)\s+" | ||
r"SK=\s*([\d\-\.E\+]+)" | ||
) | ||
ionic_general_pattern = re.compile(r"(\w+)=\s*(\S+)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the improved readability of this new regex but it's also less specific. Just wondering if there can be any parsing edge cases from not matching column headers like E0
exactly. Are there any unit tests we could add for extra safety?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think \w+ matches one or more word characters (same as [a-zA-Z0-9_]+ ) so it should not be a problem? I haven't seen any other strange OSZICAR pattern before (based on my novice experience of vasp 😂)
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #3146 +/- ##
==========================================
- Coverage 74.62% 74.06% -0.56%
==========================================
Files 230 230
Lines 69403 69394 -9
Branches 16161 16160 -1
==========================================
- Hits 51794 51399 -395
- Misses 14533 14954 +421
+ Partials 3076 3041 -35
☔ View full report in Codecov by Sentry. |
I tried to read
mag
from OSZICAR and foundmag
is not read fromionic_MD_pattern
. Not sure ifmag
is available in all vasp md calculation.@janosh should I add unit test for this? The OSZICAR in
test_files/OSZICAR
doesn't have all the available output fields likeChecklist
ruff
.mypy
.