Clean up VASP powerups, correct params in MP flows, test for `_set_u_params`, test for `_set_kspacing`, fix `_get_incar` method of `VaspInputSet` #561

esoteric-ephemera · 2023-10-10T19:32:56Z

Closes #465.

Summary

A few key changes:

Added new function, update_vasp_input_generators to vasp.powerups to remove repeated code. All subsequent functions, such as update_user_incar_settings now rely on this base function
Fixed broken logic in VaspInputGenerator._get_incar: in _get_incar, _set_kspacing was called with previous_incar is None which is never true, because previous_incar = previous_incar or {} at the top of _get_incar. More, this would imply that no previous incar was found, which means from_prev in _set_kspacing was its inverse. Should be previous_incar != {} to indicate the presence of a previous calc
After some discussion with @Andrew-S-Rosen , we noticed that vasp.sets.base._get_kspacing should return min(kspacing, 0.44), to ensure that if kspacing <= 0.22, it would not be increased to 0.44
Also from discussion with @Andrew-S-Rosen , vasp.sets.base._set_kspacing should default to Gaussian smearing (ISMEAR = 0) rather than tetrahedron (ISMEAR = -5) for a gapped material. While the forces should be consistent between these smearing methods for an insulator, my benchmarking has shown they can differ for semi-metals / narrow gapped systems. Gaussian is probably a safer choice overall
Added a unit test test_set_u_params to tests.vasp.test_sets to close issue #465
Added a unit test test_set_kspacing to tests.vasp.test_sets

codecov · 2023-10-10T19:45:42Z

Codecov Report

Merging #561 (3398395) into main (2843d04) will decrease coverage by 0.20%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #561      +/-   ##
==========================================
- Coverage   75.65%   75.45%   -0.20%     
==========================================
  Files          80       80              
  Lines        6729     6711      -18     
  Branches      996      992       -4     
==========================================
- Hits         5091     5064      -27     
- Misses       1331     1342      +11     
+ Partials      307      305       -2

Files	Coverage Δ
src/atomate2/vasp/powerups.py	`92.00% <100.00%> (-3.35%)`	⬇️
src/atomate2/vasp/sets/base.py	`75.06% <100.00%> (+0.49%)`	⬆️

... and 1 file with indirect coverage changes

janosh · 2023-10-11T03:01:30Z

@esoteric-ephemera @Andrew-S-Rosen Found different kspacing logic in pymatgen MPScanRelaxSet and changed it to the new one in this PR. I also added better test coverage. You may want to port those tests over to atomate2:

for bandgap, kspacing in ((10, 0.44), (3, 0.4136617), (1.1, 0.3064757), (0.5, 0.2832948), (0, 0.22)):
    incar = MPScanRelaxSet(struct, bandgap=bandgap).incar
    assert incar["KSPACING"] == approx(kspacing, abs=1e-5)
    assert incar["ISMEAR"] == -5 if bandgap > 1e-4 else 2
    assert incar["SIGMA"] == 0.05 if bandgap > 1e-4 else 0.2

esoteric-ephemera · 2023-10-11T16:47:05Z

Just added a test for _set_kspacing in tests/vasp/test_sets.py. There was some broken logic in VaspInputGenerator._get_incar that I fixed as well.

utf · 2023-10-11T16:52:37Z

Thanks for this. Just one comment.

Also from discussion with @Andrew-S-Rosen , vasp.sets.base._set_kspacing should default to Gaussian smearing (ISMEAR = 0) rather than tetrahedron (ISMEAR = -5) for a gapped material. While the forces should be consistent between these smearing methods for an insulator, my benchmarking has shown they can differ for semi-metals / narrow gapped systems. Gaussian is probably a safer choice overall

I think you have this backwards. ISMEAR = -5 is very important for narrow-gapped systems. If we know the system has a gap, then we should always use ISMEAR = -5.

utf · 2023-10-11T16:50:51Z

src/atomate2/vasp/sets/base.py

@@ -1125,7 +1125,7 @@ def _set_kspacing(
                incar["ISMEAR"] = user_incar_settings.get("ISMEAR", 2)
            else:
                incar["SIGMA"] = user_incar_settings.get("SIGMA", 0.05)
-                incar["ISMEAR"] = user_incar_settings.get("ISMEAR", -5)
+                incar["ISMEAR"] = user_incar_settings.get("ISMEAR", 0)


Please change this back.

I will let @esoteric-ephemera comment on the ISMEAR setting, but independent of that comment, if we do stick with ISMEAR = -5, the SIGMA should be removed.

I actually think SIGMA should stay. If ISMEAR = -5 it will be ignored, but if the number of k-points generated is less than 4, then custodian will turn ISMEAR to 0, and at this point having a small sigma will be important.

Oh that's subtle!

I think you have this backwards. ISMEAR = -5 is very important for narrow-gapped systems. If we know the system has a gap, then we should always use ISMEAR = -5.

That's true for energies but not for forces. Looking at about 370 structures from ICSD that aren't in MP, the deviations between the forces using ISMEAR = -5 and 0 persists for even narrow-gapped materials, see below.

I guess my question is if the _set_kspacing function is supposed to be MP-specific or agnostic. If it's agnostic, then I'll just update ISMEAR / SIGMA for the MP flows and revert this back to -5.

SCF convergence was the wrong phrase - I meant the reduction in needed k-point density. Both methods should help with that for semi-/metals.

I'll change this back. There's a larger ongoing discussion in MP about whether to abandon ISMEAR = -5 for relaxations in insulators (the current default) in favor of ISMEAR = 0. That will require running "fully-converged" calculations for this same set.

There's a larger ongoing discussion in MP about whether to abandon ISMEAR = -5 for relaxations in insulators (the current default) in favor of ISMEAR = 0.

Ok, I would strongly advise against doing that! There are basically no downsides to using ISMEAR = -5 for insulators and several potential downsides to using ISMEAR = 0.

That will require running "fully-converged" calculations for this same set.

What do you mean by "fully-converged"

Fully converged as in using a very high density of k-points, very high cutoffs, and no smearing (Gaussian + very small smearing). See here

Wrt downsides: I can see issues where for very small gaps, you get erroneous partial occupancies near the fermi level. Practically, the gap has to be <~ 0.15 eV for the Gaussian smearing we use (SIGMA = 0.05 by default) for virtual states to be significantly partially occupied. Are there other downsides you're thinking of?

Thanks for sharing that paper. Looks interesting.

Are there other downsides you're thinking of?

To be honest, partial occupancies is the main one. The nice thing is that if you use ISMEAR = -5 for all gapped materials then at least your calculations are then consistent between relaxation, static, and DOS. But this is obviously a very minor point.

Can I ask why you're thinking of moving away from ISMEAR = -5?

Two major reasons: since we already have a big transition in going from atomate --> atomate2, we're auditing the parameters MP uses in its WFs (esp. GGA); and there are a few collaborations on dataset generation where this ISMEAR benchmark was requested

janosh · 2023-10-12T16:49:11Z

@esoteric-ephemera Could you submit e0186dc in a separate PR?

…leanup of vasp.powerups

… in vasp.sets._get_incar

…_kspacing_and_auto_ismear since it uses kspacing by default

janosh changed the title ~~Clean up VASP powerups, correct params in MP flows, test for _set_u_params (issue #465)~~ Clean up VASP powerups, correct params in MP flows, test for _set_u_params Oct 11, 2023

janosh added testing Test all the things fix Bug fix PR house-keeping Formatting and code quality tweaks labels Oct 11, 2023

janosh mentioned this pull request Oct 11, 2023

Mirror atomate2 fix and improve MPScanRelaxSet.test_kspacing edge case coverage materialsproject/pymatgen#3396

Merged

esoteric-ephemera changed the title ~~Clean up VASP powerups, correct params in MP flows, test for _set_u_params~~ Clean up VASP powerups, correct params in MP flows, test for _set_u_params, test for _set_kspacing, fix _get_incar method of VaspInputSet Oct 11, 2023

utf reviewed Oct 11, 2023

View reviewed changes

Aaron Kaplan and others added 5 commits October 18, 2023 14:34

Test for vasp.sets.base._set_u_params, fixes to r2SCAN subroutines, c…

e4ef168

…leanup of vasp.powerups

Add test for test_set_kspacing to vasp/test_sets and fix broken logic…

5714755

… in vasp.sets._get_incar

refactor test_set_kspacing

05e91dd

Fix _set_kspacing test

b7f9e1c

fix test_set_kspacing_and_auto_ismear

a9389d3

janosh force-pushed the main branch from 9c4b2e7 to 73d47a0 Compare October 18, 2023 21:38

janosh enabled auto-merge October 18, 2023 21:38

use MPMetaGGARelaxSetGenerator instead of StaticSetGenerator test_set…

3398395

…_kspacing_and_auto_ismear since it uses kspacing by default

janosh force-pushed the main branch from 73d47a0 to 3398395 Compare October 18, 2023 22:03

janosh merged commit 4e3d156 into materialsproject:main Oct 18, 2023
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up VASP powerups, correct params in MP flows, test for `_set_u_params`, test for `_set_kspacing`, fix `_get_incar` method of `VaspInputSet` #561

Clean up VASP powerups, correct params in MP flows, test for `_set_u_params`, test for `_set_kspacing`, fix `_get_incar` method of `VaspInputSet` #561

esoteric-ephemera commented Oct 10, 2023 •

edited

Loading

codecov bot commented Oct 10, 2023 •

edited

Loading

janosh commented Oct 11, 2023

esoteric-ephemera commented Oct 11, 2023

utf commented Oct 11, 2023

utf Oct 11, 2023

Andrew-S-Rosen Oct 11, 2023

utf Oct 11, 2023

Andrew-S-Rosen Oct 11, 2023

esoteric-ephemera Oct 11, 2023

esoteric-ephemera Oct 11, 2023

utf Oct 11, 2023 •

edited

Loading

esoteric-ephemera Oct 11, 2023 •

edited

Loading

utf Oct 12, 2023 •

edited

Loading

esoteric-ephemera Oct 12, 2023

janosh commented Oct 12, 2023

Clean up VASP powerups, correct params in MP flows, test for _set_u_params, test for _set_kspacing, fix _get_incar method of VaspInputSet #561

Clean up VASP powerups, correct params in MP flows, test for _set_u_params, test for _set_kspacing, fix _get_incar method of VaspInputSet #561

Conversation

esoteric-ephemera commented Oct 10, 2023 • edited Loading

Summary

codecov bot commented Oct 10, 2023 • edited Loading

Codecov Report

janosh commented Oct 11, 2023

esoteric-ephemera commented Oct 11, 2023

utf commented Oct 11, 2023

utf Oct 11, 2023

Choose a reason for hiding this comment

Andrew-S-Rosen Oct 11, 2023

Choose a reason for hiding this comment

utf Oct 11, 2023

Choose a reason for hiding this comment

Andrew-S-Rosen Oct 11, 2023

Choose a reason for hiding this comment

esoteric-ephemera Oct 11, 2023

Choose a reason for hiding this comment

esoteric-ephemera Oct 11, 2023

Choose a reason for hiding this comment

utf Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

esoteric-ephemera Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

utf Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

esoteric-ephemera Oct 12, 2023

Choose a reason for hiding this comment

janosh commented Oct 12, 2023

Clean up VASP powerups, correct params in MP flows, test for `_set_u_params`, test for `_set_kspacing`, fix `_get_incar` method of `VaspInputSet` #561

Clean up VASP powerups, correct params in MP flows, test for `_set_u_params`, test for `_set_kspacing`, fix `_get_incar` method of `VaspInputSet` #561

esoteric-ephemera commented Oct 10, 2023 •

edited

Loading

codecov bot commented Oct 10, 2023 •

edited

Loading

utf Oct 11, 2023 •

edited

Loading

esoteric-ephemera Oct 11, 2023 •

edited

Loading

utf Oct 12, 2023 •

edited

Loading