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

Updating the Materials Project VASP parameters #25

Closed
esoteric-ephemera opened this issue Apr 19, 2024 · 24 comments
Closed

Updating the Materials Project VASP parameters #25

esoteric-ephemera opened this issue Apr 19, 2024 · 24 comments
Labels
rfc Request for comments

Comments

@esoteric-ephemera
Copy link

esoteric-ephemera commented Apr 19, 2024

Problem

I've been working to systematically audit and benchmark a few parameters and POTCAR choices made by the Materials Project used in VASP calculations. I'm linking my recommendations for how these should be updated moving forward for longevity of the next round of MP calcs

This is in anticipation of recomputing the entirety of MP with r2SCAN and adding missing ICSD structures to MP. The bulk of the evidence is provided in that document, I'm happy to provide any further data for support.

Any changes to the MP flows are reflected in mp24 modules in this fork of atomate2

I've also been chatting with @mkhorton about this benchmarking effort, and he recommended performing a k-points check. I'm in the process of devising and running this check, but wanted to ensure this topic was broached prior to the upcoming foundation meeting, since there's enough in there to discuss now.

Also tagging in @matthewkuner, @janosh, @Andrew-S-Rosen, and @rkingsbury, who have all been involved in previous benchmark efforts, or this one.

@JaGeo
Copy link
Member

JaGeo commented Apr 19, 2024

Maybe unrelated: but are there any plans for comparing input and output structures of the optimization. I have seen a few issues (e.g., with solid iodine or bromine) where the structures did suffer from a DFT error after optimization. I would love an RMS value showing the difference in fractional coordinates or similar

@esoteric-ephemera
Copy link
Author

Hi @JaGeo, happy to add a comparison for this. Are you mostly interested in cases where the spacegroup changes under relaxation (estimate of frequency, chemical spaces where this happens), or atoms with too-close/too-far separations after a relaxation?

Also for clarity: to isolate the effect of changing one parameter, most of the tests in this work involve statics for the actual comparison

@JaGeo
Copy link
Member

JaGeo commented Apr 19, 2024

I think also drastic position changes. But this might be harder to quantify than spacegroup

I think the ground state structure of solid iodine and bromine are currently not in MP anymore. The icsd IDs are matched to a structure that is far away from the experimental one, if I haven't oberlooked anything. Here, the space group stayed the same but positions changed drastically.
See here
https://next-gen.materialsproject.org/materials/mp-23153?formula=I2

Same for Br2

@Andrew-S-Rosen
Copy link
Member

Thank you SO much for this writeup! Somewhat miraculously, I agree with everything here. I am looking forward to the switch being flipped! 😁

@Andrew-S-Rosen
Copy link
Member

Andrew-S-Rosen commented Apr 20, 2024

@esoteric-ephemera --- should you put the new input sets in pymatgen? Can atomate2 accept them yet?

@janosh
Copy link
Member

janosh commented Apr 21, 2024

excellent write up! very excited for the rerun. this will allow everyone who needs to maintain MP compatibility to start using up-to-date PSPs.

minor issue: fig. 9 seems to have wrong caption.

@janosh janosh added the rfc Request for comments label Apr 21, 2024
@Andrew-S-Rosen
Copy link
Member

Andrew-S-Rosen commented Apr 21, 2024

Any reason to include "LWAVE": True, "LCHARG": False in MP24GGARelaxSetGenerator, MP24GGAStaticSetGenerator, or MP24PBEsolPreRelaxSetGenerator if those are already being set by _BASE_MP24_RELAX_SET? I'd suggest removing them from the generators so we don't have things specified in multiple places.

@Andrew-S-Rosen
Copy link
Member

Andrew-S-Rosen commented Apr 21, 2024

I suppose I have one question. It seems that we are somewhat arbitrarily updating some POTCARs from https://www.nature.com/articles/s42254-023-00655-3. Is there any reason to not just go all-in for consistency and adopt the pseudopotentials used in https://www.nature.com/articles/s42254-023-00655-3? I see in the document it says we are taking a "don't fix what isn't broken approach," which is fair, but selecting 12 to update seems like it could introduce questions later on. Personally, I don't have a vote one way or another, but I am curious about the decision.

@mkhorton
Copy link
Member

@Andrew-S-Rosen I believe the pseudopotentials in that paper are mostly the v54? I think the "don't fix what isn't broken approach" is typically wise, but if there is time for an across-the-board refresh, this would be it, especially if no regressions are observed.

@esoteric-ephemera
Copy link
Author

@janosh:

minor issue: fig. 9 seems to have wrong caption.

Fig. 9 has the wrong figure! My bad - fixed.

@mkhorton @Andrew-S-Rosen:

@esoteric-ephemera --- should you put the new input sets in pymatgen? Can atomate2 accept them yet?

That will get migrated to pymatgen. First thing is to make the transition on the atomate2 side. Will do that this week.

Any reason to include "LWAVE": True, "LCHARG": False in MP24GGARelaxSetGenerator, MP24GGAStaticSetGenerator, or MP24PBEsolPreRelaxSetGenerator if those are already being set by _BASE_MP24_RELAX_SET? I'd suggest removing them from the generators so we don't have things specified in multiple places.

There's going to be a little rearrangement of the tags so those are mostly reminders for me. The fork is very much WIP

Is there any reason to not just go all-in for consistency and adopt the pseudopotentials used in https://www.nature.com/articles/s42254-023-00655-3?

Like you said, I'm trying only to reduce the errors to a level that's acceptable, and making changes to reflect that. Right now, it seems like that level of acceptable error due to the pseudopotential is ~2.5% (see Tb, where the MAPE in $V_0$ is reduced to 2.2% using the recommended POTCAR).

That being said, there is a practical issue to using all the recommended POTCARs. The O POTCAR has an ENMAX of 765.519 eV, which would require ENCUT $\gtrsim 920$ eV for relaxations of oxides (20% inflation of ENMAX). That's going to make high-throughput with r2SCAN impossible.

I agree with @mkhorton that it wouldn't hurt to know how the recommended POTCARs perform, excluding the O_h_GW potential

Also worth noting that it seems like maybe there's a mix of versions in the recommended potentials? Except for the Ba, Cs, Cu, He, Hf, and Rn POTCARs, all of the POTCAR SHAs indicate PBE_64 POTCARs. Those 6 match the PBE_54 hashes (could also be those were not updated at all going from 54 to 64).

@janosh
Copy link
Member

janosh commented Apr 22, 2024

Also worth noting that it seems like maybe there's a mix of versions in the recommended potentials? Except for the Ba, Cs, Cu, He, Hf, and Rn POTCARs, all of the POTCAR SHAs indicate PBE_64 POTCARs. Those 6 match the PBE_54 hashes (could also be those were not updated at all going from 54 to 64).

maybe @sudarshanv01 knows more about that?

@sudarshanv01
Copy link

Hi @janosh and @esoteric-ephemera, the changelog of the potentials which were updated are shown on the portal in the text near the release (or in log.message in the folder) . I am not sure which type of potential you are using for Ba (for example) - if it isn't the GW recommended version then I don't believe it has been updated. Cross checking against that list might be the easiest way to see what is updated.

@JaGeo
Copy link
Member

JaGeo commented Apr 23, 2024

@esoteric-ephemera should I add this discussion than as the second point on our agenda for the next meeting? In preparation, @esoteric-ephemera, we should probably send out the document in the mail. I will try to do this latest on next Monday.

Follow up question on mine before: will you start the new runs from the GGA results or from ICSD? This might influence the outcome quite a bit.

@esoteric-ephemera
Copy link
Author

@JaGeo that'd be great if we set aside some time at the foundation meeting to discuss this. Hope to have some k-point density tests ready by then.

Follow up question on mine before: will you start the new runs from the GGA results or from ICSD? This might influence the outcome quite a bit.

This hasn't been decided yet, but I would imagine for any structure where the space group changed from the original input structure, we'd rerun from the original structure

@JaGeo
Copy link
Member

JaGeo commented Apr 23, 2024

This hasn't been decided yet, but I would imagine for any structure where the space group changed from the original input structure, we'd rerun from the original structure
Thanks!
Just to add: See Iodine example above. The space group change alone might not be enough. There, the structure changed (i.e., the positions) but the space group stayed the same. Might be something to think about. Maybe, another matching algorithm could be use that includes positional changes beyond those that are connected to space group changes

@JaGeo
Copy link
Member

JaGeo commented Apr 23, 2024

This hasn't been decided yet, but I would imagine for any structure where the space group changed from the original input structure, we'd rerun from the original structure

Thanks!
Just to add: See Iodine example above. The space group change alone might not be enough. There, the structure changed (i.e., the positions) but the space group stayed the same. Might be something to think about. Maybe, another matching algorithm could be use that includes positional changes beyond those that are connected to space group changes

@esoteric-ephemera
Copy link
Author

Hi all, just noting that I've updated the notes with a KSPACING / kpoint density test. I'll update the POTCAR stuff as soon as the remaining calcs finish.

@mkhorton
Copy link
Member

mkhorton commented May 3, 2024

Thank you for all the discussion so far. Can I propose that this now be made into a draft PR following the template of the other PRs? This would be the next step to prepare for a decision.

I know this might seem a bit premature since benchmarking is ongoing, but the PR would be a good place to continue the discussion following the next Foundation meeting, as well as to discuss practical matters, e.g. factors affecting a rollout, how to notify the community, etc.

@JaGeo
Copy link
Member

JaGeo commented May 3, 2024

Yes, @mkhorton , this is a great suggestion. @esoteric-ephemera would you be able to draft this for Monday? I will notify everyone about our new agenda for Monday. We will discuss on this after the Matminer discussion (in the second half of the meeting).

@esoteric-ephemera
Copy link
Author

Yep! Is it OK if I leave the PDF as is / link to a living doc in google drive? I'll rewrite the summary in markdown / link to the full doc

@janosh
Copy link
Member

janosh commented May 3, 2024

I'll rewrite the summary in markdown

you could use pandoc to auto-convert from latex to MD (or even Typst, big fan! 😄 )

@JaGeo
Copy link
Member

JaGeo commented May 3, 2024

Yep! Is it OK if I leave the PDF as is / link to a living doc in google drive? I'll rewrite the summary in markdown / link to the full doc

I guess the link plus summary is fine. 😄 And, thank you!

@esoteric-ephemera
Copy link
Author

pandoc did pretty well but complex tex is a little too much for it in this case

@esoteric-ephemera
Copy link
Author

esoteric-ephemera commented May 3, 2024

Closed by #26, please add any further discussion to that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfc Request for comments
Projects
None yet
Development

No branches or pull requests

6 participants