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

Mulit element POTCAR loading broken. #2895

Closed
MichaelWolloch opened this issue Mar 15, 2023 · 3 comments
Closed

Mulit element POTCAR loading broken. #2895

MichaelWolloch opened this issue Mar 15, 2023 · 3 comments

Comments

@MichaelWolloch
Copy link
Contributor

Describe the bug
Since commit e4bde8e it seems that loading is broken if any but the first POTCAR in the file has a sha256 hash.

I really appreciate the effort by @shyuep to clean up the code and get rid of the regex stuff and unicode handling. However, a small mistake appears to have slipped in in line 2277 of pymatgen/io/vasp/inputs.py. There should be a unicode end of line character still:

potcar_strings = fdata.split("End of Dataset\n")

instead of

potcar_strings = fdata.split("End of Dataset")

If that is included, also the test that got modified in the commit can be changed back it seems.

However, since this all seems a bit fragile, I suggest to actually also downgrade the ValueError that is raised in line 1829 in pymatgen/io/vasp/inputs.py to a warning.

To Reproduce
load the attached POTCAR (containing O and Fe_pv_with_hash from pymatgen/test_files/POT_GGA_PAW_PBE_54 with the Fe Potcar following the O) into a Potcar object:
POTCAR_test.gz

from pymatgen.io.vasp.inputs import Potcar
pot = Potcar.from_file('POTCAR_test.gz')

results in:

ValueError: POTCAR with symbol Fe_pv and functional
PBE has a SHA256 hash defined,
but the computed hash differs.
YOUR POTCAR FILE HAS BEEN CORRUPTED AND SHOULD NOT BE USED!

Expected behavior
There should be no error for the SHA256, since the POTCAR is good.

Environment (please supply relevant versions and platform info):

  • OS: Linux
  • Version 2023.3.10

Additional context
I am happy to make a pull request if this is wanted.

@shyuep shyuep closed this as completed in 53267fb Mar 15, 2023
shyuep added a commit that referenced this issue Mar 15, 2023
@e-kwsm
Copy link
Contributor

e-kwsm commented Sep 28, 2023

This is not fixed yet (2023.9.25).

~/.local/lib/python3.11/site-packages/pymatgen/io/vasp/inputs.py:1806: UnknownPotcarWarning: POTCAR data with symbol O does not match any VASP POTCAR known to pymatgen. There is a possibility your POTCAR is corrupted or that the pymatgen database is incomplete.
  warnings.warn(
~/.local/lib/python3.11/site-packages/pymatgen/io/vasp/inputs.py:1813: UserWarning: POTCAR with symbol Fe_pv and functional
PBE has a SHA256 hash defined,
but the computed hash differs.
YOUR POTCAR FILE HAS BEEN CORRUPTED AND SHOULD NOT BE USED!
  warnings.warn(

passed_hash_check = self.hash_sha256_from_file == self.hash_sha256_computed

The hashes of POTCAR_test.gz in the OP are 5d22e414b1f82158bf2c7ecb8b97b28fd0923e48cadc1c3bf74d524558f5dd32 and 52459ef0a41f1e32977d35179f8b492c701fda3d7e2f9b778c5c2267c5f2dbc0.
The former is correct:

$ grep SHA256 Fe_pv/POTCAR
   SHA256 =  5d22e414b1f82158bf2c7ecb8b97b28fd0923e48cadc1c3bf74d524558f5dd32 Fe_pv/POTCAR
$ sed -e /COPYR/d -e /SHA256/d Fe_pv/POTCAR | sha256sum -
5d22e414b1f82158bf2c7ecb8b97b28fd0923e48cadc1c3bf74d524558f5dd32  -

The latter is however strangely computed, which can be reproduced by

$ { sed -e /COPYR/d -e /SHA256/d -e '/^ *PAW_PBE/s/^ *//' -e '/^ *End of/s/^ *//' Fe_pv/POTCAR; printf ''; } | sha256sum -
52459ef0a41f1e32977d35179f8b492c701fda3d7e2f9b778c5c2267c5f2dbc0  -

@MichaelWolloch
Copy link
Contributor Author

Hi @e-kwsm !

AFAIK there were some more changes to this since March. There is a pending PR that should fix this along another thing: #3204.

However, POTCAR validation will likely be changed completely anyhow, making this a mute problem. See PR #3351.

@e-kwsm
Copy link
Contributor

e-kwsm commented Oct 3, 2023

I am not sure #3351 intends to address this, or checksum will be dropped in future, but anyway SHA256 computation is still (7cc629d) broken (#2895 (comment)).

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

No branches or pull requests

2 participants