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

Add missing MPSCANRelaxSet.yaml parameters and alphabetize #3615

Merged
merged 4 commits into from
Feb 16, 2024

Conversation

Andrew-S-Rosen
Copy link
Member

@Andrew-S-Rosen Andrew-S-Rosen commented Feb 9, 2024

Tagging @esoteric-ephemera for review, @utf for context, and @shyuep & @mkhorton as code owners of the MP sets.

This PR ensures that the MPSCANRelaxSet.yaml in Pymatgen matches the BaseMPR2SCANRelaxSet.yaml used in Atomate2 (and thereby, in MP production calculations). Namely, the MPSCANRelaxSet.yaml in pymatgen was missing the "sensible defaults" added by @esoteric-ephemera, which includes: ISMEAR: 0, KSPACING: 0.22, SIGMA: 0.05. Everything else is just a reordering for alphabetization purposes.

@Andrew-S-Rosen Andrew-S-Rosen changed the title Add missing MPSCANRelaxSet.yaml parameters Add missing MPSCANRelaxSet.yaml parameters and alphabetize Feb 9, 2024
@Andrew-S-Rosen
Copy link
Member Author

@esoteric-ephemera --- I see the thumbs up. Does that mean this is good to go, or are you still planning to review this?

@mkhorton
Copy link
Member

Looks good to me. I think ISMEAR was probably omitted since this was intended to be set dynamically based on gap. I have no objection to include a default value.

I believe there was also some talk about switching LREAL to False, but this might require a separate, updated input set, since it would technically break compatibility with the published input set.

@esoteric-ephemera
Copy link
Contributor

@Andrew-S-Rosen thanks for putting this together! @mkhorton, yeah for the new input sets, putting them in a separate PR would be good. I'll reach out to the foundation soonish with that

@Andrew-S-Rosen
Copy link
Member Author

Great, thank you both! I also agree that some minor modifications should be made but that will be elsewhere.

@shyuep, I think we have the necessary blessings for this one.

@Andrew-S-Rosen
Copy link
Member Author

@janosh should I update the hash?

@janosh
Copy link
Member

janosh commented Feb 16, 2024

yes please

@janosh janosh enabled auto-merge (squash) February 16, 2024 13:34
@janosh janosh added io Input/output functionality fix Bug fix PRs vasp Vienna Ab initio Simulation Package ecosystem Concerning the larger pymatgen ecosystem labels Feb 16, 2024
@janosh janosh merged commit 170c701 into materialsproject:master Feb 16, 2024
22 checks passed
@Andrew-S-Rosen Andrew-S-Rosen deleted the r2scanparams branch February 16, 2024 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ecosystem Concerning the larger pymatgen ecosystem fix Bug fix PRs io Input/output functionality vasp Vienna Ab initio Simulation Package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants