Breaking: New method of POTCAR validation by esoteric-ephemera · Pull Request #3351 · materialsproject/pymatgen

esoteric-ephemera · 2023-09-26T16:57:20Z

Summary

Issues:
current method of POTCAR validation is too brittle to work well in high-throughput calculations. For example, hashes are sensitive to subtle changes in spaces (addition/deletion). However FORTRAN is space-insensitive, thus two POTCARs that differ by spaces are read identically by VASP.

Goals:
Develop less brittle method for validating POTCARs that reflects how POTCAR metadata and numeric values are read by FORTRAN.

Major changes:

is_valid function in pymatgen.io.vasp.inputs.PotcarSingle, which checks POTCAR metadata and values against a database of these
To respect copyright, only statistics on the numerical values are stored separately for the PSCTR / POTCAR header and pseudopotential numeric values
Keywords found within the PSCTR header and pseudopotential body are stored as well for comparison. For example, older POTCARs may have lacked the kinetic energy density (needed for meta-GGA calcs), and were later updated to include this. The check distinguishes the older and newer version of the same POTCAR
Tests have been updated (TestPotcarSingle class in tests/io/vasp/test_inputs.py)

Todos

While I generated POTCAR metadata from those available to our group, might be beneficial to expand the database of POTCAR metadata to older / newer releases

mkhorton · 2023-09-26T18:02:52Z

Thanks for taking a fresh look at this problem @esoteric-ephemera, it's been unsatisfactory for a while. These warnings frequently confuse or alarm people unnecessarily, but the underlying motivation of rigorously checking pseudopotential compatibility is of course very important.

However FORTRAN is space-insensitive, thus two POTCARs that differ by spaces are read identically by VASP.

I imagine there's a way around this, to normalize the POTCAR file, but it might be slow and perhaps there'd still be brittleness in other ways so I like your suggestions.

When the original POTCAR hash database was assembled we tried to find as many examples as we could, but I'm still surprised there are files out there that might differ by e.g. spaces within the file itself, since I can't imagine why these edits would have been made.

mkhorton · 2023-09-26T18:03:59Z

+            return input_str
+
+    def is_valid(self, tol: float = 1.0e-6) -> bool:
+        """Check that POTCAR matches reference metadata."""


Could you expand this docstring to specify what is being checked (e.g., what attributes of PotcarSingle), and also what the tol is doing, and why this can be changed by the user? It seems like a fixed tolerance might be preferred.

Yes! Might have gone a little too verbose in explaining what's going on in is_valid(), but there's a better explanation there now.

For "tol", I left that adjustable for debugging - it's frozen now at the same value

esoteric-ephemera · 2023-09-26T21:26:14Z

Thanks both @janosh and @mkhorton! Tried to address your comments.

It is possible just to completely strip a POTCAR of spaces and hash it that way, but I see one issue with that: in the PSCTR / header section of the POTCAR, there are comments. Normally, in fortran, in-line comments would be indicated by a leading "!", but these are omitted in the proprietary POTCAR format. One could add comments without changing any numeric values, and get the same POTCAR

There was actually a test of PotcarSingle in PMG that did just that - it basically added comments to a PSCTR tag. A hash would report these as different, but the numeric values are identical to vasp

janosh

Nice work @esoteric-ephemera! 👍

I think we just need to rename test_identify_potcar and call is_valid in there.

janosh · 2023-09-27T01:30:28Z

+        if input_str.lower() in ["t", "f"] or input_str.lower() in ["true", "false"]:
+            return input_str[0].lower() == "t"
+
+        if (input_str.upper() == input_str.lower()) and input_str[0].isnumeric():


what about floats like 1e3? I guess they never appear in POTCARs?

Fortran-style scientific format always includes a decimal point on write, like 1.0E-06 or 0.5E+2, see this guide. While you can write 1E4, you can't format a float without a decimal point in fortran.

Probably best to rename this to _fortran_style_str_to_py_var

janosh · 2023-09-27T01:35:04Z

-        assert "Fe" in psingle.identify_potcar()[1]
+        matched_funcs = [refpsp["POTCAR_FUNCTIONAL"] for refpsp in psingle._matched_meta]
+        assert "PBE_54" in matched_funcs
+        assert "Fe" in psingle._matched_meta[0]["TITEL"]


Tests generally shouldn't test an implementation detail like this private _matched_meta list. We need a test called test_is_valid that only calls the public PotcarSingle.is_valid property both for a case where it's True and False.

Done! And thanks for cleaning up my messy python. The new test_is_valid operates on a working Fe POTCAR, and one where one variable is changed by 0.001. Should highlight that the check truly is sensitive to individual numeric values.

Andrew-S-Rosen · 2023-09-27T04:42:53Z

Thank you, @esoteric-ephemera, for helping me sleep better at night with this PR.

…fy_potcar to test_is_valid to ensure that PotcarSingle.is_valid is not sensitive to small numeric changes

…R on the fly

janosh · 2023-09-27T19:34:11Z

+
+        # corrupt the file
+        assert psingle.keywords["RCORE"] == 2.3
+        psingle.keywords["RCORE"] = 2.2
+        assert not psingle.is_valid

-    def test_potcar_hash_warning(self):
+        psingle.keywords.pop("RCORE")
+        assert not psingle.is_valid
+
+        psingle.data = psingle.data.replace("RCORE  =    2.300", "RCORE  =    2.200")
+        assert not psingle.is_valid
+
+    def test_unknown_potcar_warning(self):


I think rather than storing a new POTCAR file with just 1 changed line, it should be easy to invalidate a PotcarSingle instance on the fly. But actually this turned out to be pretty hard. You might say this is a contrived example since people shouldn't change PotcarSingle objects once they've been read from a file. But fwiw, I can currently reassign

psingle.keywords["RCORE"] = 2.2

remove it altogether

psingle.keywords.pop("RCORE")

and modify the underlying string data

psingle.data = psingle.data.replace("RCORE = 2.300", "RCORE = 2.200")

all without invalidating the PotcarSingle instance. See the new asserts I added. I think it would make sense for all of them to pass but they currently fail.

This is because the header info is taken from self.PSCTR and not the parse_functions attributes of PotcarSingle. For each such attribute like RCORE (the ones in self.parse_functions), the key/value are stored both in self.keywords and self.PSCTR

So if you do instead self.PSCTR['RCORE'] = 2.2, the second psingle.is_valid will fail. Or, if you did
psingle.data = psingle.data.replace('local part','local port')
the second psingle.is_valid will also fail.

Hesitant to change how this is done (storing the same data in three different attributes of PotcarSingle) because it might break something that relies on it

Hesitant to change how this is done (storing the same data in three different attributes of PotcarSingle) because it might break something that relies on it

I don't think we need to be hesitant here. having redundant data in PotcarSingle.keywords and PotcarSingle.PSCTR is needlessly complicated, goes against the SSOT coding principle and now would be a great time to clean this up imo.
@shyuep @mkhorton Any concerns about reworking PotcarSingle to e.g. remove PSCTR and turn keywords into a @property so they're computed on the fly and always in sync with the underlying data string?

I agree, having redundant data doesn't make sense for the reasons you state. My only hesitation here is that I wouldn't want to impose scope creep on this PR for @esoteric-ephemera, since it's probably not strictly necessary to change it for this PR to achieve its goal of better validation. While the PotcarSingle should be immutable, I think the risk of someone mutating it in practice are quite low.

More broadly, I've never had a good understanding of the internals of the POTCAR file. If anyone did want to spend more time on the PotcarSingle class, I'd love to see more documented @property etc. for some of these keywords so that people can better understand what the POTCAR files they're using are actually specifying.

I merged PotcarSingle.keywords and PotcarSingle.PSCTR, now just called PotcarSingle.keywords since PSCTR has no obvious meaning. Maybe we should rename PotcarSingle.keywords to PotcarSingle.header_keywords for added clarity?

mkhorton · 2023-09-27T22:36:57Z

At the risk of asking a stupid question, I saw the MPRelaxSet defaults changed Yb_2 to Yb_3 recently (see #2968).

However, as far as I can tell, only Yb_2 is in the POTCAR database for the PBE functional (i.e., the MPRelaxSet default). The Yb_3 only exists for the PBE_52/PBE_54.

So my questions are, (1) is this correct?, and (2), if so, is it even possible to run MPRelaxSet with Yb-containing materials at present?

janosh · 2023-09-27T22:58:29Z

So my questions are, (1) is this correct?, and (2), if so, is it even possible to run MPRelaxSet with Yb-containing materials at present?

@mkhorton I think you forgot about #2972 which reverted the Yb_2 -> Yb_3 migration for MPRelaxSet:

pymatgen/pymatgen/io/vasp/MPRelaxSet.yaml

Line 175 in 8e0b099

Yb: Yb_2

mkhorton · 2023-09-27T23:05:36Z

Indeed I did, thanks @janosh! This was not reverted in atomate2 so the two sets are currently inconsistent (I realise this is an active topic of discussion right now).

Apologies to @esoteric-ephemera for bringing a tangent to this PR.

fixes TestTransformedStructure.test_get_vasp_input > return self.keywords[attr.upper()] E KeyError: 'HASH' (was broken in 983598)

trying to fix tests

janosh · 2023-09-29T18:34:48Z

                "but the computed hash differs.\n"
-                "YOUR POTCAR FILE HAS BEEN CORRUPTED AND SHOULD NOT BE USED!"
+                "YOUR POTCAR FILE HAS BEEN CORRUPTED AND SHOULD NOT BE USED!",
+                UnknownPotcarWarning,


@esoteric-ephemera @mkhorton @Andrew-S-Rosen Are we sure we want to keep the old unknown POTCAR warning when the SHA256 hash provided by VASP doesn't match the one computed from the file? Isn't that what has been giving so many false positive warnings?

Does this PR not make this more robust, reducing false positives?

The warning is definitely badly phrased if we do keep it, it’s inscrutable unless someone is very familiar with VASP and pymatgen. We could link to a docs page for explanation.

Does this PR not make this more robust, reducing false positives?

The new warning that's supposed to be more robust comes just above these lines:

pymatgen/pymatgen/io/vasp/inputs.py

Lines 1778 to 1783 in c329ff5

if not has_sha256 and not self.is_valid:

warnings.warn(

f"POTCAR data with symbol {self.symbol} is not known to pymatgen. Your "

"POTCAR may be corrupted or pymatgen's POTCAR database is incomplete.",

UnknownPotcarWarning,

)

Currently, when not self.is_valid is False, i.e. the POTCAR is valid according to the new validation, then we still check whether computed and in-file hash match and trigger the old warning if not (which has many false positives, I think):

elif has_sha256 and not sha256_pass: warnings.warn( f"POTCAR with symbol {self.symbol} and functional\n" ...

The SHA56 warning is not what's been giving us so many UnknownPotcarWarnings, that was the internal PMG hashing which has been removed. Some versions of VASP POTCARs have a SHA56 tag in the header that VASP computed on their side. We don't have any SHA56-tagged POTCARs for me to test that the current validator works well in their place

Though honestly, given the comments on issue #2895, it seems like the SHA56 has similar fragility as PMG's hashes. I would lean towards removing the SHA56 check

given the comments on issue #2895, it seems like the SHA56 has similar fragility as PMG's hashes

that was my understanding as well

I would lean towards removing the SHA56 check

same!

rename potcar_file_hash to md5_computed_file_hash rename potcar_hash to md5_header_hash

…self.sha256_computed_file_hash prone to false positives

…lid example in test_is_valid instead

…d_file_hash

janosh

I think this is great! 2 things in this PR:

less brittle way of checking for invalid POTCARs
clean up of PotcarSingle class. now less confusing due to
- no duplication between keywords and (the no longer existent) PSCTR
- using @property methods for hashes instead of direct class attributes

Thanks @esoteric-ephemera!

Aaron Kaplan and others added 11 commits September 22, 2023 16:33

Added two funcs to PMG's PotcarSingle to better check POTCAR meta

4ef3913

updates to gen new meta for potcar validation

e6a9b42

use only req'd potcar meta, and not full contents of psctr header

ee5cafa

use only req'd potcar meta, and not full contents of psctr header 2/*

0525ddd

Use PSCTR instead of keywords attr to generate meta

983598e

Use PSCTR instead of keywords attr to generate meta

23cd75b

First attempt at new POTCAR hashing ready, tests pass

7991fae

Cleanup and prep for PR

e066d5f

rm tmp.diff

f789f98

gzip POTCAR_META.json

d4f15b4

auto-format vasp/inputs.py

15075b6

mkhorton reviewed Sep 26, 2023

View reviewed changes

janosh and others added 3 commits September 26, 2023 11:18

Merge branch 'master' into esoteric-ephemera/master

b492cba

pre-commit auto-fixes

12189c4

Updating in response to PMG maintainer comments

1ea1804

janosh added 4 commits September 26, 2023 18:23

deduplicate cwd and rename module_dir

394f133

make is PotcarSingle.is_valid property

b72f476

remove newlines from warning

be7ffb7

snake_case

d33e2ab

janosh suggested changes Sep 27, 2023

View reviewed changes

Aaron Kaplan and others added 3 commits September 27, 2023 09:26

rename _str_to_py to _fortran_style_str_to_py_var; update test_identi…

1f815a3

…fy_potcar to test_is_valid to ensure that PotcarSingle.is_valid is not sensitive to small numeric changes

tweak UnknownPotcarWarning msg

f533081

rm POT_GGA_PAW_PBE_54/POTCAR.Fe_broken.gz and try to invalidate POTCA…

f80160f

…R on the fly

janosh reviewed Sep 27, 2023

View reviewed changes

janosh added 3 commits September 28, 2023 15:55

reintroduce PotcarSingle.hash and file_hash attrs

a0ded26

fixes TestTransformedStructure.test_get_vasp_input > return self.keywords[attr.upper()] E KeyError: 'HASH' (was broken in 983598)

delete alias test_dir_doscar for TEST_FILES_DIR

ab5ae17

fix test_get_all_possible_basis_functions

9505f8e

janosh added io Input/output functionality vasp Vienna Ab initio Simulation Package breaking Breaking change api Application programming interface ux User experience labels Sep 29, 2023

janosh added 4 commits September 29, 2023 09:29

fix TestMPScanRelaxSet.test_write_input polluting git repo on failure

b695942

git checkout master tests/files/POT_GGA_PAW_PBE_54/POTCAR.{Fe_pv,O}.gz

89c74b6

trying to fix tests

mv pymatgen/io/vasp/{POTCAR_META,potcar_summary_stats}.json.gz

4a20c93

move _fortran_style_str_to_py_var into def is_valid()

c329ff5

janosh reviewed Sep 29, 2023

View reviewed changes

janosh added 9 commits September 29, 2023 11:35

rename PotcarSingle.sha256_file_hash to sha256_computed_file_hash

933f1c1

missed a hash_sha256_computed->sha256_computed_file_hash rename

d1eede9

add PotcarSingle property hash_sha256_from_file

1c35748

rename potcar_file_hash to md5_computed_file_hash rename potcar_hash to md5_header_hash

remove old unknown Potcar warning when self.hash_sha256_from_file != …

14d6515

…self.sha256_computed_file_hash prone to false positives

delete test_potcar_file_hash_warning, use that potcar test file as va…

8ae3eca

…lid example in test_is_valid instead

remove aliases hash for md5_header_hash and file_hash for md5_compute…

7a19eaa

…d_file_hash

load pymatgen and VASP hashes only once in module scope

20bd485

git checkout master tests/files/POT_GGA_PAW_PBE_54/POTCAR.Fe_O.gz

8ba2a4c

remove vestiges of potcar_hash alias for md5_computed_file_hash

6900a02

janosh changed the title ~~New method of POTCAR validation~~ Breaking: New method of POTCAR validation Sep 29, 2023

janosh approved these changes Sep 30, 2023

View reviewed changes

janosh merged commit f114f2d into materialsproject:master Sep 30, 2023

janosh mentioned this pull request Oct 2, 2023

Fix pymatgen breaking change: PotcarSingle.get_potcar_hash renamed to md5_header_hash materialsproject/emmet#839

Merged

esoteric-ephemera mentioned this pull request Oct 2, 2023

Update POTCAR summary stats to include 6.4 POTCARs and add dev_script utils for future updates #3370

Merged

janosh mentioned this pull request Oct 10, 2023

Further updating POTCAR validation / identification #3392

Merged

This was referenced Oct 28, 2023

Add summary_stats attr to emmet.core.vasp.calculation.PotcarSpec materialsproject/emmet#874

Merged

Add summary_stats key to Vasprun.potcar_spec #3434

Merged

	if not has_sha256 and not self.is_valid:
	warnings.warn(
	f"POTCAR data with symbol {self.symbol} is not known to pymatgen. Your "
	"POTCAR may be corrupted or pymatgen's POTCAR database is incomplete.",
	UnknownPotcarWarning,
	)

Uh oh!

Conversation

esoteric-ephemera commented Sep 26, 2023

Summary

Todos

Uh oh!

mkhorton commented Sep 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esoteric-ephemera commented Sep 26, 2023

Uh oh!

janosh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esoteric-ephemera Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esoteric-ephemera Sep 27, 2023 • edited by janosh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andrew-S-Rosen commented Sep 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janosh Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkhorton commented Sep 27, 2023

Uh oh!

janosh commented Sep 27, 2023

Uh oh!

mkhorton commented Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janosh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

esoteric-ephemera Sep 27, 2023 •

edited

Loading

esoteric-ephemera Sep 27, 2023 •

edited by janosh

Loading

janosh Sep 27, 2023 •

edited

Loading

mkhorton commented Sep 27, 2023 •

edited

Loading

janosh left a comment •

edited

Loading