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

esoteric-ephemera · 2023-10-02T21:35:44Z

Major changes:

Follow-up to issue #3342 and closed PR #3351
Adds support for version 64 POTCARs
Adds hidden function for PMG devs to continuously update potcar_summary_stats.json.gz used to validate potcars, pymatgen.io.vasp.inputs._gen_potcar_summary_stats
Added functionality to generate fake POTCARs from real POTCARs (by randomizing numeric and boolean values in them) for tests in dev_scripts/potcar_scrambler.py

…_stats file used in validation

janosh · 2023-10-02T21:40:33Z

pymatgen/io/vasp/inputs.py

+def _gen_potcar_summary_stats():
+    """
+    This function solely intended to be used for PMG development to regenerate the
+    potcar_summary_stats.json.gz file used to validate POTCARs
+    """


Perfect! This is great to have. Even though it's marked as private, we should add a test for this.

Added a Test_gen_potcar_summary_stats to tests/io/vasp/test_inputs.py. Requires generating a library of fake POTCARs, which are in tests/fake_POTCAR_library. Code to generate this library from a real library of POTCARs is in dev_scripts/potcar_scrambler.py

mkhorton · 2023-10-02T21:42:30Z

pymatgen/io/vasp/inputs.py

@@ -2280,6 +2283,43 @@ def __repr__(self) -> str:
        return f"{cls_name}({symbol=}, {functional=}, {TITEL=}, {VRHFIN=}, {n_valence_elec=:.0f})"


+def _gen_potcar_summary_stats():


This is fine here, but noting there is also a top-level dev_scripts folder (largely undocumented). On balance it's probably better here for discoverability for anyone curious how this file was generated.

Minor comments: the bare return is unnecessary, and monty.serialization.dumpfn can be used directly with dumpfn(new_summary_stats, "filename.json.gz") which is a convenience function we use a lot.

Thanks! Much better way of doing this. For placement, you and @janosh probably have a better sense of where this belongs. _gen_potcar_summary_stats is a "destructive" function, in that it overwrites the file that's distributed with PMG. dev_scripts might then be a better place for it

…TCARs from existing POTCARs by randomizing the data contained in them, dev_scripts/potcar_scrambler.py; (3) unit test for pymatgen.io.vasp.inputs._gen_potcar_summary_stats by generating summary stats for a library of fake POTCARs and then checking that the fake set passes PotcarSingle.is_valid with overriden stats

…p") with os.makedirs(exist_ok=True)

…P_PSP_DIR"]

janosh

Really nice work, thanks @esoteric-ephemera! 👍

lan496 · 2023-10-04T02:36:00Z

Hi. I think pymatgen/io/vasp/potcar_summary_stats.json.gz should be added to setup.package_data to use it via pip-installed version.

pymatgen/setup.py

Lines 95 to 119 in 44bf383

    
           package_data={ 
        
               "pymatgen.analysis": ["*.yaml", "*.json", "*.csv"], 
        
               "pymatgen.analysis.chemenv": [ 
        
                   "coordination_environments/coordination_geometries_files/*.json", 
        
                   "coordination_environments/coordination_geometries_files/*.txt", 
        
                   "coordination_environments/strategy_files/ImprovedConfidenceCutoffDefaultParameters.json", 
        
               ], 
        
               "pymatgen.analysis.structure_prediction": ["*.yaml", "data/*.json"], 
        
               "pymatgen.analysis.diffraction": ["*.json"], 
        
               "pymatgen.analysis.magnetism": ["default_magmoms.yaml"], 
        
               "pymatgen.analysis.solar": ["am1.5G.dat"], 
        
               "pymatgen.entries": ["*.json.gz", "*.yaml", "data/*.json"], 
        
               "pymatgen.core": ["*.json"], 
        
               "pymatgen": ["py.typed"], 
        
               "pymatgen.io.vasp": ["*.yaml", "*.json"], 
        
               "pymatgen.io.feff": ["*.yaml"], 
        
               "pymatgen.io.cp2k": ["*.yaml"], 
        
               "pymatgen.io.lobster": ["lobster_basis/*.yaml"], 
        
               "pymatgen.command_line": ["*"], 
        
               "pymatgen.util": ["structures/*.json", "*.json"], 
        
               "pymatgen.vis": ["*.yaml"], 
        
               "pymatgen.io.lammps": ["CoeffsDataType.yaml", "templates/*.template"], 
        
               "pymatgen.symmetry": ["*.yaml", "*.json", "*.sqlite"], 
        
               "cmd_line": ["**/*"], 
        
           },

janosh · 2023-10-04T02:48:18Z

Ah crap!

thanks #3370 (comment)

mkhorton · 2023-10-04T03:04:11Z

Not the first time don’t worry! I once added a catch-all to package all .json, .yaml files by default but this was vetoed for reasons I forget, but perhaps worth reconsidering

….package_data` (#3372) * fix bare URLS they don't work in https://pymatgen.org/CHANGES.html * add pymatgen/io/vasp/potcar_summary_stats.json.gz to setup.package_data thanks #3370 (comment)

janosh · 2023-10-04T15:21:07Z

I once added a catch-all to package all .json, .yaml files by default but this was vetoed for reasons I forget, but perhaps worth reconsidering

I think it's worth a try. @shyuep Any concerns?

If we get false positives, we can always revert the change.

shyuep · 2023-10-04T15:49:50Z

My concern is that data files should be few and far between and EXPLICIT.

janosh · 2023-10-04T15:53:20Z

My concern is that data files should be few and far between and EXPLICIT.

Exactly, which is why the ones we deliberately place in the pymatgen pkg dir should be there for a good reason and hence also be included in the PyPI release. At least that's the case right now as you can see from d8151e5 passing.

shyuep · 2023-10-04T15:58:06Z

Yes, that is why it should be explicitly specified. For the same reason, you do not usually do from package import *.

janosh · 2023-10-04T16:06:35Z

The biggest reason not to do from package import * imo is that it breaks tooling. IDEs can trace where things come from so no doc string glancing, no type hints and worse code completions. But none of those apply here, there's no tooling for pkg data anyway. So far, there's been a 1-to-1 map from data files in pymatgen to things included in SOURCES.txt so we might as well make the map automatic imo.

shyuep · 2023-10-04T16:45:29Z

There are reasons. Say someone decides to add a very_big_file.yaml for some internal testing purposes. That file will automatically be added to package data, even if it has no real purpose in actual function of the main code. Like I said, it is better to be explicit what files are needed and what are not.

janosh · 2023-10-04T16:48:02Z

I don't merge very big files into master. Not even in test files, let alone in pkg dir. Test files should never be placed in pkg dir.

shyuep · 2023-10-04T16:49:24Z

You don't != Someone else don't. Anyway, we don't add data files all the time. So just leave it as is. I don;'t want to go round and round on an issue that has occurred once in two years.

Aaron Kaplan and others added 4 commits October 2, 2023 13:43

Added option for 64 POTCARs, hidden func to regenerate potcar_summary…

afbbf79

…_stats file used in validation

Updated potcar_summary_stats.json.gz to include 64 POTCARs

4a84723

Verify new POTCAR_64 summary_stats work; prep for PR

f3b62f8

pre-commit auto-fixes

5828944

janosh suggested changes Oct 2, 2023

View reviewed changes

janosh added enhancement A new feature or improvement to an existing one io Input/output functionality vasp Vienna Ab initio Simulation Package labels Oct 2, 2023

mkhorton reviewed Oct 2, 2023

View reviewed changes

janosh and others added 12 commits October 2, 2023 20:24

Merge branch 'master' into master

404bbb9

pre-commit auto-fixes

66cfac8

cleanup for ruff/lint/etc

cca9d39

google-style doc str

ac2408d

replace print with warnings.warn

ca5d50d

refactor test_gen_potcar_summary_stats using pytest fixtures

b955eeb

rename function arg PMG_VASP_PSP_DIR to vasp_psp_dir

5c54cd0

cleanup fake potcar library to only include a few required examples

93d3bcd

replace os.system('rm -rf') with shutil.rmtree() and system(f"mkdir -…

a916a68

…p") with os.makedirs(exist_ok=True)

git mv tests/files/fake_{POTCAR,potcar}_library

6ace41b

generate_fake_potcar_libraries prefix src_dirs with SETTINGS["PMG_VAS…

209c0b0

…P_PSP_DIR"]

janosh approved these changes Oct 3, 2023

View reviewed changes

janosh changed the title ~~Updating POTCAR summary stats to include 64 version POTCARs~~ Update POTCAR summary stats to include 6.4 POTCARs and add dev_script utils for future updates Oct 3, 2023

janosh enabled auto-merge (squash) October 3, 2023 23:00

fix overlooked rename fake_(POTCAR->potcar)_library

baef777

janosh disabled auto-merge October 3, 2023 23:11

janosh merged commit 8c594d7 into materialsproject:master Oct 3, 2023

janosh added a commit that referenced this pull request Oct 4, 2023

add pymatgen/io/vasp/potcar_summary_stats.json.gz to setup.package_data

7946198

thanks #3370 (comment)

janosh mentioned this pull request Oct 4, 2023

Fix missing potcar_summary_stats.json.gz in setup.package_data #3372

Merged

janosh mentioned this pull request Oct 4, 2023

Fix outdated setup.py find_namespace_packages and add test_egg_sources_txt_is_complete #3374

Merged

janosh mentioned this pull request Oct 10, 2023

Further updating POTCAR validation / identification #3392

Merged

esoteric-ephemera mentioned this pull request Oct 20, 2023

Support VASP 64 pseudopotentials #3342

Closed

1 task

esoteric-ephemera mentioned this pull request Feb 13, 2024

Improve INCAR tag check #3621

Merged

esoteric-ephemera mentioned this pull request Sep 17, 2024

Adding PBE_64 support: Clarification on hash types used in POTCAR hash files. #3956

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

esoteric-ephemera commented Oct 2, 2023 •

edited

Loading

janosh Oct 2, 2023

esoteric-ephemera Oct 3, 2023

mkhorton Oct 2, 2023

esoteric-ephemera Oct 2, 2023

janosh left a comment

lan496 commented Oct 4, 2023

janosh commented Oct 4, 2023

mkhorton commented Oct 4, 2023

janosh commented Oct 4, 2023

shyuep commented Oct 4, 2023

janosh commented Oct 4, 2023

shyuep commented Oct 4, 2023

janosh commented Oct 4, 2023

shyuep commented Oct 4, 2023

janosh commented Oct 4, 2023

shyuep commented Oct 4, 2023

		@@ -2280,6 +2283,43 @@ def __repr__(self) -> str:
		return f"{cls_name}({symbol=}, {functional=}, {TITEL=}, {VRHFIN=}, {n_valence_elec=:.0f})"


		def _gen_potcar_summary_stats():

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

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

Conversation

esoteric-ephemera commented Oct 2, 2023 • edited Loading

Major changes:

janosh Oct 2, 2023

Choose a reason for hiding this comment

esoteric-ephemera Oct 3, 2023

Choose a reason for hiding this comment

mkhorton Oct 2, 2023

Choose a reason for hiding this comment

esoteric-ephemera Oct 2, 2023

Choose a reason for hiding this comment

janosh left a comment

Choose a reason for hiding this comment

lan496 commented Oct 4, 2023

janosh commented Oct 4, 2023

mkhorton commented Oct 4, 2023

janosh commented Oct 4, 2023

shyuep commented Oct 4, 2023

janosh commented Oct 4, 2023

shyuep commented Oct 4, 2023

janosh commented Oct 4, 2023

shyuep commented Oct 4, 2023

janosh commented Oct 4, 2023

shyuep commented Oct 4, 2023

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

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

esoteric-ephemera commented Oct 2, 2023 •

edited

Loading