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

Breaking: Default user_potcar_settings to {"W": "W_sv"} in all input sets if user_potcar_functional == "PBE_54" #3022

Merged
merged 8 commits into from
Jun 1, 2023

Conversation

janosh
Copy link
Member

@janosh janosh commented May 30, 2023

Closes #3016, closes #1707.

Breaking: DictSet drop deprecated kwarg potcar_functional: str = None, use user_potcar_functional: str = None instead (in ac51e26)

@janosh janosh added io Input/output functionality vasp Vienna Ab initio Simulation Package ux User experience labels May 30, 2023
@janosh janosh requested a review from shyuep May 30, 2023 21:12
@janosh
Copy link
Member Author

janosh commented May 30, 2023

Pinging @JaGeo and @arosen93 for review.

@janosh janosh added the breaking Breaking change label May 30, 2023
@janosh janosh changed the title Default user_potcar_settings to {"W": "W_sv"} in all input sets if user_potcar_functional == "PBE_54" Breaking: Default user_potcar_settings to {"W": "W_sv"} in all input sets if user_potcar_functional == "PBE_54" May 30, 2023
@janosh
Copy link
Member Author

janosh commented May 30, 2023

Actually, the PR title is inaccurate. There are input sets that don't have any restrictions on user_potcar_settings which I didn't touch. So far, I only changed the input sets that ensure user_potcar_functional in ("PBE_52", "PBE_54"). So would make sense to move this behavior up in the class hierarchy into DictSet where it would always apply.

in kwargs["user_potcar_settings"] = {"W": "W_sv"}.update(kwargs.get("user_potcar_settings", {}))

pymatgen/io/vasp/sets.py:2352

also address #3022 (comment) by moving {"W": "W_sv"} logic into DictSet
for DRY ValueError on invalid user_potcar_functional
@JaGeo
Copy link
Member

JaGeo commented May 31, 2023

@janosh Currently, this code here, does not work:

from pymatgen.io.vasp.sets import MPRelaxSet
from pymatgen.core.structure import Structure

structure = Structure.from_dict({'@module': 'pymatgen.core.structure', '@class': 'Structure', 'charge': 0.0,
                                 'lattice': {'matrix': [[2.6794492400614325, 0.0, -1.7723083848423267],
                                                        [7.778511770202987e-16, 4.837013999999999,
                                                         2.9618168562654676e-16], [0.0, 0.0, 5.76034817]],
                                             'pbc': (True, True, True), 'a': 3.21255743, 'b': 4.837013999999999,
                                             'c': 5.76034817, 'alpha': 90.0, 'beta': 123.48244503999997, 'gamma': 90.0,
                                             'volume': 74.65718535099107}, 'sites': [
        {'species': [{'element': 'W', 'oxidation_state': 4.0, 'occu': 1.0}], 'abc': [0.0, 0.5, 0.0],
         'xyz': [3.8892558851014935e-16, 2.4185069999999995, 1.4809084281327338e-16], 'label': 'W4+', 'properties': {}},
        {'species': [{'element': 'W', 'oxidation_state': 4.0, 'occu': 1.0}], 'abc': [0.0, 0.0, 0.5],
         'xyz': [0.0, 0.0, 2.880174085], 'label': 'W4+', 'properties': {}},
        {'species': [{'element': 'O', 'oxidation_state': -2.0, 'occu': 1.0}], 'abc': [0.292339, 0.302264, 0.7959505],
         'xyz': [0.7833075113903194, 1.4620551996959996, 4.066837145169164], 'label': 'O2-', 'properties': {}},
        {'species': [{'element': 'O', 'oxidation_state': -2.0, 'occu': 1.0}], 'abc': [0.707661, 0.802264, 0.7040495],
         'xyz': [1.896141728671114, 3.880562199695999, 2.8013767249885095], 'label': 'O2-', 'properties': {}},
        {'species': [{'element': 'O', 'oxidation_state': -2.0, 'occu': 1.0}], 'abc': [0.707661, 0.697736, 0.2040495],
         'xyz': [1.8961417286711137, 3.3749588003039994, -0.0787973600114904], 'label': 'O2-', 'properties': {}},
        {'species': [{'element': 'O', 'oxidation_state': -2.0, 'occu': 1.0}], 'abc': [0.292339, 0.197736, 0.2959505],
         'xyz': [0.7833075113903193, 0.9564518003039998, 1.1866630601691643], 'label': 'O2-', 'properties': {}}]})
mprelax = MPRelaxSet(structure=structure, user_potcar_functional="PBE_54")
mprelax.write_input("input")

The current change does not seem to correctly fix the issue.

@JaGeo
Copy link
Member

JaGeo commented May 31, 2023

Maybe user_potcar_settings = {"W": "W_sv"}.update(user_potcar_settings) if user_potcar_settings is not None else {"W": "W_sv"} instead? Probably there is a more compact form.

@janosh
Copy link
Member Author

janosh commented May 31, 2023

Hey @JaGeo, thanks for taking a look! 🙏

Good catch! Didn't want to write tests until initial feedback but I probably should have noticed this without tests. 😅

{"W": "W_sv"}.update(user_potcar_settings)

returns None is the problem. It should have been

user_potcar_settings = {"W": "W_sv", **(user_potcar_settings or {})}

Fix and test incoming.

@janosh
Copy link
Member Author

janosh commented May 31, 2023

@JaGeo I removed self._config_dict["POTCAR"].update({"W": "W_sv"}) from LobsterSet since that's now handled for all subclasses of DictSet. Is that ok?

@janosh janosh merged commit d25319a into master Jun 1, 2023
41 checks passed
@janosh janosh deleted the fix-gh-3016 branch June 1, 2023 00:41
@JaGeo
Copy link
Member

JaGeo commented Jun 1, 2023

@JaGeo I removed self._config_dict["POTCAR"].update({"W": "W_sv"}) from LobsterSet since that's now handled for all subclasses of DictSet. Is that ok?

@janosh yes, completely fine!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change io Input/output functionality ux User experience vasp Vienna Ab initio Simulation Package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

InputSet looks for wrong W POTCAR when using PBE_54 W_pv not included in newer POTCAR versions
2 participants