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.

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

Fix LDA pseudo parsing and related parsing bugs #1207

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

esoteric-ephemera
Copy link
Collaborator

@esoteric-ephemera esoteric-ephemera commented Mar 14, 2025

Closes #847: parsing LDA pseudopotential calcs failed using TaskDoc. Also fixed another bug that would render TaskDoc.input.pseudo_potentials as:

Potcar(pot_type='PAW', functional='P_B_E', symbols=['PAW_PBE'])

rather than

Potcar(pot_type='PAW', functional='PBE', symbols=['Si'])

(double-checked: the tasks on MP all have the former Potcar style if that field is populated).

Both of the previous parsing issues stem from the fact that we don't parse the majority of POTCAR info from the POTCAR, but the vasprun.xml. Parsing directly from POTCAR would let us access these attributes from the individual PotcarSingle objects (potential_type, LEXCH, and symbol, respectively)

I can change the parsing to prefer POTCAR-derived attributes if that's desirable moving forward. Also forward-looking in case we change to parsing from vaspout.h5, which contains the full POTCAR as a str

@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.15%. Comparing base (947ecc8) to head (162d632).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1207      +/-   ##
==========================================
+ Coverage   90.13%   90.15%   +0.01%     
==========================================
  Files         147      147              
  Lines       14506    14509       +3     
==========================================
+ Hits        13075    13080       +5     
+ Misses       1431     1429       -2     

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tschaume
Copy link
Member

Thanks @esoteric-ephemera ! PR looks good and is ok to merge I think.

I think I'm in favor of parsing directly from the POTCAR in the future. Couple of questions to clarify:

  • Are there any scenarios in which the info from the POTCAR could be different from what's saved in the vasprun at the end of a calculation?
  • Would we be able to parse the PotcarSingle from the original POTCAR as well as the non-proprietary POTCAR hash we'll be saving on OpenData?
  • We would probably also have to remove the full POTCAR string and replace with the hash in vaspout.h5, right? Is there any proprietary POTCAR info stored in the current vasprun.xml?

@esoteric-ephemera
Copy link
Collaborator Author

Are there any scenarios in which the info from the POTCAR could be different from what's saved in the vasprun at the end of a calculation?

The info should be the same as in vasprun.xml, but there's less info stored in vasprun.xml overall. I think it's just the "TITEL" fields stored in vasprun.xml and the element symbols

Would we be able to parse the PotcarSingle from the original POTCAR as well as the non-proprietary POTCAR hash we'll be saving on OpenData?

Some of the POTCAR hashes are the same because they weren't updated in between releases (like between PBE_52 and PBE_54 releases) so sort of - we can give a set of guesses for what the POTCAR was.

In the new validation scheme for POTCARs, we build a subset of POTCAR hashes that are acceptable, and use those to validate a guess POTCAR or its hash. That gets around the issue of needing to ID the POTCAR uniquely

We would probably also have to remove the full POTCAR string and replace with the hash in vaspout.h5, right? Is there any proprietary POTCAR info stored in the current vasprun.xml

We do but there's a feature in the vaspout.h5 parser I added to pymatgen that does just that. vasprun.xml has no copyrighted info

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug when parsing LDA pseudopotential
3 participants