[FIX] Fix major bug that caused user_incar_settings to be overwritten in some cases

[matthewkuner]

Summary

Fixes major bug that caused user_incar_settings to be overwritten in some cases.

[matthewkuner]

@utf Let me know your thoughts on setting auto_ismear to False, as this will affect whether or not I have to change the tests. Otherwise, this is ready to be merged.

[utf]

Sorry for the late response. There is an issue with just applying user_incar_settings again at the end. That's because some settings like MAGMOM/LDAUU etc are set using a dictionary in the user_incar_settings. These are then parsed by the input set generator and converted into the format required by VASP.

So I think we'll have to do it the way I mentioned in the GitHub issue.

[matthewkuner]

@utf no problem. And what are your thoughts regarding having auto_ismear as False?

[utf]

Personally, I think we can leave it to True just for consistency. But maybe @jmmshn has an opinion since he implemented this change.

[matthewkuner]

@jmmshn Honestly I am lost with how to fix the failing tests for ISMEAR. Can you help me with this?

[jmmshn]

Hi @matthewkuner I can only recall implementing auto_lreal I think auto_ismear was done with the major update to the VASP settings earlier this year.

As far as fixing the failing tests it just looks like you have changed settings to the incars are now different.
I think you have three options:

1. Rerun the tests with the updated settings
2. Edit the inputs and outputs manually (INCAR, OUTCAR, (maybe) vasprun.xml) but this will might create inconsistencies in parsing.
3. Mock only a small part of the test to avoid this problem.

I would start with (2), see how intrusive the change becomes, and then make decisions from there.

[utf]

If you go for point 2, you only need to edit the input INCAR (not any of the outputs).

Edit, actually you shouldn't have to edit anything. This looks like the behaviour of the input set has changed?

[matthewkuner]

@utf could this be related to materialsproject/custodian#271 ?

[matthewkuner]

@jmmshn is there any chance you could help figure out what is going wrong here? I'm honestly lost w.r.t. understanding the mock vasp / whether the files should be changed or if it was a change in the input set behavior (as mentioned by @utf) / etc.

[matthewkuner]

@utf

This looks like the behaviour of the input set has changed?

The relevant files in the MPInputSets haven't changed in months. Do you mean the Atomate2 sets?

[matthewkuner]

@jmmshn Could this have to do with the changes made to _set_kspacing in your #270 ?

[jmmshn]

@matthewkuner OK I can help. There are some weird stuff breaking for me as well so it will make sense to have a chat.
Let's coordinate offline.

[utf]

Ok, this seems fixed now. @matthewkuner would you be able to add an extra test for the previous behaviour that was broken?

[codecov]

Codecov Report

Merging #412 (8288eb8) into main (02e44c0) will increase coverage by 0.57%.
Report is 235 commits behind head on main.
The diff coverage is 90.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #412      +/-   ##
==========================================
+ Coverage   64.96%   65.54%   +0.57%     
==========================================
  Files          74       78       +4     
  Lines        7196     7479     +283     
  Branches      946      970      +24     
==========================================
+ Hits         4675     4902     +227     
- Misses       2221     2275      +54     
- Partials      300      302       +2     

Files Changed	Coverage Δ
src/atomate2/vasp/sets/base.py	69.47% <90.00%> (+0.24%)	⬆️

... and 33 files with indirect coverage changes

[matthewkuner]

@utf tests have been added. Let me know if they are adequate--if so, this should be ready for merging.

[utf]

Great, thank you!