Skip to content
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.

Sign up for GitHub

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

Jump to bottom

read mag from OSZICAR #3146

Merged
merged 23 commits into from
Aug 18, 2023
Merged

read mag from OSZICAR #3146

merged 23 commits into from
Aug 18, 2023

Conversation

chiang-yuan
Copy link
Contributor

@chiang-yuan chiang-yuan commented Jul 11, 2023 •
edited
Loading

I tried to read mag from OSZICAR and found mag is not read from ionic_MD_pattern. Not sure if mag 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 like

2 T=  1239. E= 0.11712678E+03 F= -.20873815E+03 E0= -.20873814E+03  EK= 0.10092E+02 SP= 0.32E+03 SK= 0.10E-02 mag=     -0.000

Checklist

  • Google format doc strings added. Check with ruff.
  • Type annotations included. Check with mypy.
  • Tests added for new features/fixes.

@janosh
Copy link
Member

janosh commented Jul 11, 2023

@chiang-yuan Yes, we definitely need a test for this.

Do I understand correctly that your PR adds a mag key to any parsed OSZICAR even if that data is not present in the file? Will the value be None in that case?

@janosh janosh added enhancement A new feature or improvement to an existing one needs testing PRs that are not ready to merge due to lacking tests io Input/output functionality vasp Vienna Ab initio Simulation Package labels Jul 11, 2023
@chiang-yuan
Copy link
Contributor Author

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

class OszicarTest(PymatgenTest):
def test_init(self):
filepath = f"{self.TEST_FILES_DIR}/OSZICAR"
oszicar = Oszicar(filepath)
assert len(oszicar.electronic_steps) == len(oszicar.ionic_steps)
assert len(oszicar.all_energies) == 60
assert oszicar.final_energy == approx(-526.63928)

@chiang-yuan chiang-yuan mentioned this pull request Jul 14, 2023
Open
@chiang-yuan
Copy link
Contributor Author

Hi @janosh I think this PR is ready to be merged. Let me know if I can do anything to make it better!

janosh
janosh suggested changes Jul 25, 2023
View reviewed changes
pymatgen/io/vasp/tests/test_outputs.py Outdated Show resolved Hide resolved
test_files/OSZICAR_MD Outdated Show resolved Hide resolved
pymatgen/io/vasp/outputs.py
r"SP=\s*([\d\-\.E\+]+)\s+"
r"SK=\s*([\d\-\.E\+]+)"
)
ionic_general_pattern = re.compile(r"(\w+)=\s*(\S+)")
Copy link
Member

@janosh janosh Jul 25, 2023 •
edited
Loading

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?

Copy link
Contributor Author

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 😂)

janosh
janosh suggested changes Jul 28, 2023
View reviewed changes
pymatgen/io/vasp/tests/test_outputs.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Aug 9, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.56% ⚠️

Comparison is base (d9fdfef) 74.62% compared to head (179d20a) 74.06%.

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
Files Changed Coverage Δ
pymatgen/io/vasp/outputs.py 88.12% <100.00%> (+0.20%) ⬆️

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@janosh janosh merged commit a1c19db into materialsproject:master Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janosh janosh janosh approved these changes

Assignees
No one assigned
Labels
enhancement A new feature or improvement to an existing one io Input/output functionality needs testing PRs that are not ready to merge due to lacking tests vasp Vienna Ab initio Simulation Package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chiang-yuan @janosh @codecov-commenter