-
Notifications
You must be signed in to change notification settings - Fork 918
Fix PBE_64
folder directory name in pymatgen.io.vasp.inputs
#4458
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
base: master
Are you sure you want to change the base?
Conversation
Closes #4456. Signed-off-by: Andrew S. Rosen <[email protected]>
PBE_64
folderPBE_64
folder directory name in pymatgen.io.vasp.sets
PBE_64
folder directory name in pymatgen.io.vasp.sets
PBE_64
folder directory name in pymatgen.io.vasp.inputs
FYI, letting you know @esoteric-ephemera. |
Did this not get fixed in #4431? |
@esoteric-ephemera: Thanks for the redirect. Yes, it looks like if the user installs Pymatgen today and runs the CLI entirely from scratch, everything should work due to #4431. Upon further inspection, it seems like the issue @jinlhr542 and I ran into in #4456 was due to the breaking change. In previous versions of Pymatgen, the CLI would make a folder called |
@esoteric-ephemera: So I guess the question is --- do we revert the breaking name change, or do we just stick with what we have in |
(I would, as usual, be happy about avoiding breaking changes. 😅) |
Either way leads to a breaking change, since a user who set up the directory structure manually and doesn't use the CLI (like me) will have it break the other way Only non breaking solution would be a check for either directory name |
@esoteric-ephemera: Yes, this is also true. I am okay with keeping things as is if that's preferred, but I want to document it here in case we get other reports. I will go ahead and close this PR, but anyone can comment if they want it re-opened. |
Hey sorry I was unclear, I'm happy to add this check for either PBE.64 folder name. It's probably for the best if both are recognized by pymatgen |
@esoteric-ephemera: That works and may reduce the headache in the long-term for MP staff 😉 Thanks for clarifying and offering to help! Certainly feel free to do so if you feel so inclined! |
Looks good to me. |
@shyuep FYI, I wouldn't merge as-is yet. @esoteric-ephemera suggested supporting both names. I'll leave that for others to deal with. |
Closes #4456.
Summary
The
PBE_64
directory name was set toPOT_PAW_PBE_64
instead ofPOT_GGA_PAW_PBE_64
inpymatgen.io.vasp.inputs
even though the Pymatgen CLI uses the latter name. This PR fixes the naming so that it matches what is produced from the CLI, which is consistent with the older POTCAR versions as well.EDIT: The new CLI as of 2025.6.14 uses
POT_PAW_PBE_64
, but this is a breaking change compared to the CLI behavior before 2025.6.14. This PR reverts the name change.