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 MP Base Sets #27

Merged
merged 3 commits into from
Dec 5, 2021
Merged

Add MP Base Sets #27

merged 3 commits into from
Dec 5, 2021

Conversation

Andrew-S-Rosen
Copy link
Member

@Andrew-S-Rosen Andrew-S-Rosen commented Dec 5, 2021

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.

@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2021

Codecov Report

Merging #27 (2c9d890) into main (f319382) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            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              
Impacted Files Coverage Δ
src/atomate2/vasp/jobs/amset.py 0.00% <0.00%> (ø)
src/atomate2/vasp/flows/amset.py 0.00% <0.00%> (ø)

@utf
Copy link
Member

utf commented Dec 5, 2021

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.

@Andrew-S-Rosen
Copy link
Member Author

Andrew-S-Rosen commented Dec 5, 2021

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.

Agreed, this seems like a good approach.

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).

Ah, yes you're right. The pseudopotential selection is different though, so would that be a problem?

Perhaps for now, can you just leave the MP GGA base yaml. And I'll add a powerup that will convert all jobs to be MP compatible.

Sure thing.

@utf
Copy link
Member

utf commented Dec 5, 2021

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.

@utf utf merged commit 4e1a0e9 into materialsproject:main Dec 5, 2021
@Andrew-S-Rosen
Copy link
Member Author

Andrew-S-Rosen commented Dec 5, 2021

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 Bi_d had convergence problems), these are either the VASP-recommended pseudopotential suffixes or ones that have a higher e- count (so are presumably more accurate anyway). As a result, I feel pretty good about them in general and wouldn't suggest any changes. Of course, it would require rerunning a fairly substantial number of r2SCAN jobs if we did ever make a change. All the more reason to keep the slightly modified set. For the BaseVaspSet.yaml used in atomate2, I think it makes sense to use the VASP-recommended potentials by default.

As a side note, the recommended pseudopotential for Yb is Yb_2. This always struck me as a bad decision because Yb is more typically Yb(III). Back in the day, the MP team even noted that Yb_2 gives poor results for thermochemistry, as discussed here, and I have observed the same with regards to band gaps. I'm not suggesting you deviate from the VASP-recommended set for consistency's sake, but I did want to bring it up. [Unfortunately, despite the MP docs saying Yb_3 is preferred, this never made its way to the MPRelaxSet.]

@mkhorton
Copy link
Member

mkhorton commented Oct 21, 2022

Sorry I missed this PR when it was opened way back, catching up on the conversation.

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.

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.

[Unfortunately, despite the MP docs saying Yb_3 is preferred, this never made its way to the MPRelaxSet.]

Not too late to change--only a few Yb materials. I would be comfortable doing a production campaign to switch specific pseudopotentials of concern. Let's discuss this offline?

[Edit: added link to pymatgen test]

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

Successfully merging this pull request may close these issues.

None yet

4 participants