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

replace check for potcar hash with check for potcar summary stats #966

Merged
merged 15 commits into from
Mar 20, 2024

Conversation

tsmathis
Copy link
Collaborator

The function .get_potcar_hash() has been removed from the PotcarSingle class in pymatgen. As a stop gap while we are migrating to new validation practices, we need to still be able to verify that the potcars used for a calculation match the pymatgen input set.

The proposed change from @esoteric-ephemera is to now check the potcar summary stats.

@tsmathis tsmathis requested a review from munrojm March 12, 2024 18:43
@tsmathis
Copy link
Collaborator Author

@esoteric-ephemera , any further comments/details you have would be helpful

@tsmathis
Copy link
Collaborator Author

tests for emmet-core/tests/vasp/ need to be updated as well once the potcar checking is finalized

@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2024

Codecov Report

Attention: Patch coverage is 60.97561% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 90.03%. Comparing base (183d74c) to head (960f19b).
Report is 3 commits behind head on main.

Files Patch % Lines
emmet-core/emmet/core/vasp/validation.py 47.05% 9 Missing ⚠️
emmet-builders/emmet/builders/utils.py 68.75% 5 Missing ⚠️
...met-builders/emmet/builders/vasp/task_validator.py 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #966      +/-   ##
==========================================
+ Coverage   90.01%   90.03%   +0.02%     
==========================================
  Files         138      138              
  Lines       13164    13182      +18     
==========================================
+ Hits        11850    11869      +19     
+ Misses       1314     1313       -1     

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

@esoteric-ephemera
Copy link
Collaborator

We discussed this a bit and decided that for now, we'll maintain the legacy hash checking when the summary_stats kwarg isn't populated in a TaskDoc / TaskDocument. This field will be populated when parsing a VASP output directory, but is not currently populated in most (probably all) of the MP tasks.

Submitted a PR to this branch to add legacy checking as a fallback when the summary_stats kwarg isn't populated for any single potcar_spec.

@munrojm munrojm merged commit cf9f606 into main Mar 20, 2024
10 checks passed
@tsmathis tsmathis deleted the update-potcar-checking branch March 20, 2024 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants