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 change in "POTCAR" info with recent changes to VaspInput #3860

Closed
Andrew-S-Rosen opened this issue Jun 4, 2024 · 7 comments
Closed
Labels
bug io Input/output functionality vasp Vienna Ab initio Simulation Package

Comments

@Andrew-S-Rosen
Copy link
Member

Andrew-S-Rosen commented Jun 4, 2024

Python version

3.11

Pymatgen version

2024.5.31

Operating system version

No response

Current behavior

It seems there was a recent breaking change to the VaspInput class that I didn't see communicated. I wanted to report it here to make sure it wasn't a bug. Feel free to close if it's intended.

Take the following example:

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

struct = Structure(
    lattice=[[0, 2.13, 2.13], [2.13, 0, 2.13], [2.13, 2.13, 0]],
    species=["Mg", "O"],
    coords=[[0, 0, 0], [0.5, 0.5, 0.5]],
)
vasp_input = MPRelaxSet().get_input_set(structure=struct, potcar_spec=True)
print(vasp_input)

In pymatgen < 2024.5.31, we get:

{'INCAR': {'ALGO': 'Fast',
  'EDIFF': 0.0001,
  'ENCUT': 520,
  'IBRION': 2,
  'ISIF': 3,
  'ISMEAR': -5,
  'ISPIN': 2,
  'LASPH': True,
  'LORBIT': 11,
  'LREAL': 'Auto',
  'LWAVE': False,
  'NELM': 100,
  'NSW': 99,
  'PREC': 'Accurate',
  'SIGMA': 0.05,
  'MAGMOM': [0.6, 0.6]},
 'KPOINTS': pymatgen with grid density = 1643 / number of atoms
 0
 Gamma
 9 9 9,
 'POSCAR': Mg1 O1
 1.0
    0.0000000000000000    2.1299999999999999    2.1299999999999999
    2.1299999999999999    0.0000000000000000    2.1299999999999999
    2.1299999999999999    2.1299999999999999    0.0000000000000000
 Mg O
 1 1
 direct
    0.0000000000000000    0.0000000000000000    0.0000000000000000 Mg
    0.5000000000000000    0.5000000000000000    0.5000000000000000 O,
 'POTCAR': ['Mg_pv', 'O']}

However, in pymatgen >=2024.5.31, we get

{'INCAR': {'ALGO': 'Fast',
  'EDIFF': 0.0001,
  'ENCUT': 520,
  'IBRION': 2,
  'ISIF': 3,
  'ISMEAR': -5,
  'ISPIN': 2,
  'LASPH': True,
  'LORBIT': 11,
  'LREAL': 'Auto',
  'LWAVE': False,
  'NELM': 100,
  'NSW': 99,
  'PREC': 'Accurate',
  'SIGMA': 0.05,
  'MAGMOM': [0.6, 0.6]},
 'KPOINTS': pymatgen with grid density = 1643 / number of atoms
 0
 Gamma
 9 9 9,
 'POSCAR': Mg1 O1
 1.0
    0.0000000000000000    2.1299999999999999    2.1299999999999999
    2.1299999999999999    0.0000000000000000    2.1299999999999999
    2.1299999999999999    2.1299999999999999    0.0000000000000000
 Mg O
 1 1
 direct
    0.0000000000000000    0.0000000000000000    0.0000000000000000 Mg
    0.5000000000000000    0.5000000000000000    0.5000000000000000 O,
 'POTCAR.spec': 'Mg_pv\nO'}

Note how doing vasp_input["POTCAR"] will cause a crash in the newer version. In its place is vasp_input["POTCAR.spec"], which also is not formatted the same way.

Tagging @esoteric-ephemera, @janosh for comment.

@Andrew-S-Rosen Andrew-S-Rosen changed the title Breaking change with recent changes to VaspInput Breaking change in "POTCAR" info with recent changes to VaspInput Jun 4, 2024
@janosh janosh added io Input/output functionality vasp Vienna Ab initio Simulation Package labels Jun 4, 2024
@janosh
Copy link
Member

janosh commented Jun 4, 2024

thanks for reporting! pretty sure this was not intentional but @esoteric-ephemera would know best.

always good to learn where we need to add more tests :)

@Andrew-S-Rosen
Copy link
Member Author

Andrew-S-Rosen commented Jun 4, 2024

It seems here is the cause of the breaking change for the key name, and here (pymatgen.io.vasp.sets.get_input_set line 458... not sure why it's not jumping to that point) is the cause of the breaking change for the data type (list[str] vs. str).

I'm not opposed to the change. Just wanted to make sure it was intentional.

@esoteric-ephemera
Copy link
Contributor

esoteric-ephemera commented Jun 4, 2024

It is intentional - I had to implement this change to get VaspInputSet to write a POTCAR.spec file instead of POTCAR. There's probably a non-breaking way to do this (label the item POTCAR in the dict, but write a file called POTCAR.spec), but I also think labeling this item as a POTCAR.spec entity rather than a POTCAR makes more sense

@janosh
Copy link
Member

janosh commented Jun 4, 2024

but I also think labeling this item as a POTCAR.spec entity rather than a POTCAR makes more sense

did you add a test for the new behavior?

@esoteric-ephemera
Copy link
Contributor

@janosh: Yes or at least modified an existing test

Both ways of doing it are fine by me, and it's an oversight on my part to not warn about this breaking things. Should I tune this a bit?

@janosh
Copy link
Member

janosh commented Jun 4, 2024

Should I tune this a bit?

personally, i think it's fine. i just added the breaking label to #3835. we could add a sentence warning of this change to CHANGES.md and https://pymatgen.org/compatibility.html#recent-breaking-changes

@Andrew-S-Rosen
Copy link
Member Author

Thanks for clarifying, @esoteric-ephemera! Feel free to close, @janosh, when you feel comfortable doing so.

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

No branches or pull requests

3 participants