-
Notifications
You must be signed in to change notification settings - Fork 102
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 MP Base Sets #27
Add MP Base Sets #27
Conversation
Codecov Report
@@ Coverage Diff @@
## main #27 +/- ##
==========================================
- Coverage 69.60% 69.56% -0.04%
==========================================
Files 44 44
Lines 3678 3680 +2
Branches 549 550 +1
==========================================
Hits 2560 2560
- Misses 973 975 +2
Partials 145 145
|
I'm not sure how's best to handle this yet. I think the easiest thing would be some kind of powerup that automatically adjusts the config dict so that the input sets are compatible with MP energies. As for the SCAN sets. I still need to port over the scan sets from pymatgen. Ultimately, I don't think there will be any incompatible changes so I don't think having the MP scan base yaml will be necessary. E.g., MPScan already uses the latest pseudo potentials and PBEsol (for the preconditioning step). Perhaps for now, can you just keep the MP GGA base yaml in this PR. And I'll add a powerup that will convert all jobs to be MP compatible. |
Agreed, this seems like a good approach.
Ah, yes you're right. The pseudopotential selection is different though, so would that be a problem?
Sure thing. |
Good point about the pseudo potential differences. I've made the atomate2 sets use all the VASP recommended pseudo potentials. I think it is worth having a conversation with Ryan whether this is worth doing in the pymatgen input sets. If there is good reason not to use the recommended pseudopotentials, then we can update atomate2 also. |
I can provide a bit of background here. In short, there is good reason. The pseudopotential suffixes used by @rkingsbury are meant to match the ones used with the GGA(+U) calculations on MP, which were originally based on some small-scale benchmarking. In nearly every case (except Bi since As a side note, the recommended pseudopotential for Yb is |
Sorry I missed this PR when it was opened way back, catching up on the conversation.
My concern about duplicating is that they introduce confusion about source of truth. In particular, we have had an incident of significant allocation waste because an input set was changed in pymatgen without the person performing calculations being notified. One additional process check we do now is actually that we test the hashes of the config files in pymatgen, so that a test explicitly fails if the config file changes, and there's a callout in that test to notify specific individuals: i.e., it introduces an additional process whereby a PR can't just be merged inadvertently that changes a config file. So, regardless, I think we should adopt that in atomate2. But I think also probably importing the MP config file from pymatgen might be the right call so there's only a single source of truth.
Not too late to change--only a few [Edit: added link to pymatgen test] |
Do you want to add MP sets in atomate2 directly, or is that something you want the user to rely on providing? On one hand, I'm a little hesitant about porting them over because it'd just be duplicating what's in
pymatgen.io.vasp.sets
. On the other hand, they'll probably be widely used. I'm curious to see what your philosophy is about this.