Reading and writing POTCAR files is broken

[henriquemiranda]

Description

Reading and writing POTCAR file with the Potcar class from pymatgen.io.vasp seems broken.

Repro

To reproduce the problem, I start with a file:

POTCAR

 PAW Sb 04Feb1998
  5.00000000000000
 [...]
  0.000000000000E+00  0.000000000000E+00  0.000000000000E+00
 End of Dataset
  PAW Te 03Oct2001
   6.00000000000000
 [...]
  0.000000000000E+00  0.000000000000E+00  0.000000000000E+00 0.000000000000E+00  0.000000000000E+00
 End of Dataset

I use the following script to copy the POTCAR:

from pymatgen.io.vasp import Potcar
potcar = Potcar.from_file('POTCAR')
potcar.write_file('POTCAR_NEW')

the resulting file becomes:
POTCAR_NEW

 PAW Sb 04Feb1998
  5.00000000000000
 [...]
  0.000000000000E+00  0.000000000000E+00  0.000000000000E+00End of Dataset
  PAW Te 03Oct2001
   6.00000000000000
 [...]
  0.000000000000E+00  0.000000000000E+00  0.000000000000E+00 0.000000000000E+00  0.000000000000E+00End of Dataset

Expected behavior

I would expect the files in POTCAR_NEW to be the same as the ones in POTCAR

Problem

The problem originated after #2910 was merged.
Perhaps @jmmshn and @janosh have some insight.
From the discussion, it looks like the relevant tests were broken by these changes and then deactivated.

I also mention @MichaelWolloch since he knows about the handling of the hashes of the POTCAR files.

[janosh]

Just to confirm, the problem is "End of Dataset" not being on a new line?

[henriquemiranda]

Yes, exactly

[jmmshn]

Thanks for reporting this!
I think the last fix on parsing fixed one white space problem and created another. I'm currently on family leave until the end of next week I can fix it when I come back.

[MichaelWolloch]

I think the problem is in reading the POTCAR from file. The .stip() method is used when constructing the PotcarSingle objects when it should not be used.

This should work (in the Potcar class):

    @staticmethod
    def from_file(filename: str):
        """
        Reads Potcar from file.

        :param filename: Filename
        :return: Potcar
        """
        with zopen(filename, "rt") as f:
            fdata = f.read()
        potcar = Potcar()

        functionals = []
        for p in fdata.split("End of Dataset\n"):
            if p.strip():
                single = PotcarSingle(p + "End of Dataset\n")
                potcar.append(single)
                functionals.append(single.functional)
        if len(set(functionals)) != 1:
            raise ValueError("File contains incompatible functionals!")
        potcar.functional = functionals[0]
        return potcar

It is essentially what was there before commit 1e9119a.

Regarding the discussion in #2910, I understand that the hash system is a bit brittle and cumbersome. But I do not think commenting out tests is the way to go. If one needs to keep the hash checks, then one should try and add more unit tests precisely because it is a fickle thing. Otherwise, one should just get rid of it entirely.

I want to stress that the issue @henriquemiranda describes does not only give a hash warning. The resulting POTCAR is not valid and will crash VASP. So this has not strictly to do with the hash system.

[jmmshn]

I think this can also work:

- single = PotcarSingle(p_strip + "End of Dataset\n")
+ single = PotcarSingle(p_strip + "\nEnd of Dataset\n")

I believe p_strip was used here to help make sure that the output POTCARs were more consistent with the hope that we implement some kind of more usable hashing in the future but that obviously backfired.

I'm not actually able to pull the code since I'm in China (on family leave) until the end of next week and the VPN has been really slow.
So if this is an emergency someone else will have to patch and test this. Apologies.

[MichaelWolloch]

This version does produce a valid POTCAR, but it still fails the implemented hash checks. I really do not see the point in reformatting POTCARs. No full file hash can work consistently for both POTCARs that are just taken from the library and maybe concatenated together, and ones that have been reformatted by pymatgen.

I took me a lot of effort to repair the hashing system that was throwing warnings every time a POTCAR was used for a long time in #2762. I tried to make the hashing system more usable and future proof by using POTCAR internal hashes as well and not only the ones in the pymatgen hash tables. I am the first to admit that the solutions were not very easy to read because of all the regex statements, but at least it worked...

[janosh]

I took me a lot of effort to repair the hashing system that was throwing warnings every time a POTCAR was used for a long time in #2762.

Apologies @MichaelWolloch. Certainly not our intent to waste anyone's time or undo work.

I really do not see the point in reformatting POTCARs.

I think @jmmshn was having trouble parsing POTCAR headers which motivated the change in #2910:

Without the fix:

pc = Potcar.from_file("./POTCAR")
{d.header for d in pc}

Gives:

{'PAW_PBE Ga_d 06Jul2010', ''}

with the fix, you get:

{'PAW_PBE Ga_d 06Jul2010', 'PAW_PBE N 08Apr2002'}

Maybe there's a middle ground here...

[MichaelWolloch]

No apologies needed @janosh , I realize of course that nobody is doing anything out of malice.
My only concern is that tests are being deleted and warnings ignored. Of course the header of each PotcarSingle should be accessible from the Potcar object.

And I would also not be mad if the hashing checks are completely removed at some point. I personally think that they have value, and that POTCARs should never be modified (as is also the VASP companies policy), but one does not necessarily have to check for modification by hashes.

The code I posted above and also gives the right headers by the way (using @henriquemiranda's example):

from pymatgen.io.vasp import Potcar

potcar = Potcar.from_file("POTCAR_old")
potcar.write_file("POTCAR")
print({d.header for d in potcar})

gives (without a warning due to a failed hash):
{'PAW Sb 04Feb1998', 'PAW Te 03Oct2001'}

[shyuep]

Just remove all that ridiculous hashing stuff already. I see zero value in them, except to cause more headaches for maintainers.

[MichaelWolloch]

Fine with me. Just be careful that the POTCARs are not modified in a way that breaks functionality as happened in #2910 in the future. I feel that hashing is a reasonable way to do this, even if it is not easy to maintain. But I am continuing to offer my help here.

[jmmshn]

Agreed there should have a basic POTCAR parsing/validation test. I'll make sure to add it when I get back.

[MichaelWolloch]

Thanks @jmmshn , I am happy to take part in discussions on how this could look without using hashes or executing VASP to see if the POTCAR is valid.