Skip final LDAU/J/L/MAGMOM updates and fix setting MAGMOM via user_incar_settings

[JonathanSchmidt1]

Skip LDAU/J/L and MAGMOM when applying user settings to final INCAR. Corrected MAGMOMS to MAGMOM

Summary

LDAU/J/L and MAGMOM tags are skipped when applying the user_incar_settings a final time to the INCAR. This fixes an issue of them being written as a dictionary in the final INCAR. The second commit corrects as spelling mistake that would cause user_incar_settings of MAGMOM to be ignored.

Closes #646.

[JonathanSchmidt1]

I assume that no new tests will be needed but test_user_incar_settings() in tests/vasp/test_sets.py would have to be adjusted?
Can someone confirm this?

[utf]

Thanks very much for this. It would be good to add a new test. One that simply creates a new input set with user_incar_settings, and then calls get_input_set()["INCAR"] and checks that the magmom/LDAU is set correctly.

[JonathanSchmidt1]

Is test_user_incar_settings() not already this function?
The LDAU/J/L could be added to the test_user_incar_settings() and the MAGMOM result could be changed. Currently, test_user_incar_settings() fails because it anticipates MAGMOM as a dictionary in the final INCAR, whereas to my understanding it should actually be formatted as a list in the final INCAR.

I am a bit confused about the input to the failed test_incar_magmoms_precedence() .
{"MAGMOM": [3.7, 0.8]} is used as user_incar_settings however _get_magmom() expects a dict[str, float] .
It appears that this list format might have been a temporary workaround for the issue we are addressing now. Previously, _get_magmom() would fail to locate the species in user_incar_settings when provided as a list, rather than a dictionary with species as keys. However the final INCAR would be overwritten by the last update, which we are now omitting for MAGMOM. This allowed the test to pass by setting MAGMOM to the list.

To get the test correct I would say the input needs to be changed to the dictionary.
Do you agree or am misunderstanding sth?

[JonathanSchmidt1]

@utf any opinions on which if any tests have to be changed/ new ones introduced?
One questions I have is whether the magmoms in user_incar_settings should be a dictionary as explained in the VaspInputGenerator documentation or a list as in the test_incar_magmoms_precedence .

[utf]

@JonathanSchmidt1, I think you're right that the test_incar_magmoms_precendence is wrong. The magmoms should be specified as a dictionary as per the VaspInputGenerator docstring.

[JonathanSchmidt1]

Locally the test in test_sets now passed. However, there is a potential issue. For structures containing spin, the MAGMOM entry in the INCAR file must be specifically formatted to work. For example:
{"MAGMOM": {"Fe2+,spin=4": 3.7, "O2-,spin=0.63": 0.8}}
This format is required because get_magmom function checks str(site.species). This wasn't initially clear to me, and it may not be obvious to other users. To improve clarity, one could consider updating the documentation or implementing a warning for cases where a user has a spin-structured MAGMOM entry in INCAR with incorrectly formatted keys.

[utf]

Thanks for catching this. I also wouldn't have known that immediately. Would you be able to information about this to the docstring for user_incar_settings on the base input set?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5ec73f7) 76.25% compared to head (a671f14) 76.10%.

❗ Current head a671f14 differs from pull request most recent head 0cf5df0. Consider uploading reports for the commit 0cf5df0 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #648      +/-   ##
==========================================
- Coverage   76.25%   76.10%   -0.16%     
==========================================
  Files          87       87              
  Lines        7150     7151       +1     
  Branches     1057     1057              
==========================================
- Hits         5452     5442      -10     
- Misses       1374     1386      +12     
+ Partials      324      323       -1     

Files	Coverage Δ
src/atomate2/vasp/sets/base.py	74.05% <100.00%> (+0.06%)	⬆️

... and 1 file with indirect coverage changes

[utf]

Hi @JonathanSchmidt1, are you able to install the pre-commit by running the following in the root atomate2 directory

pip install pre-commit
pre-commit install
pre-commit run --all

This should fix the failing linting.

[JonathanSchmidt1]

Hi @JonathanSchmidt1, are you able to install the pre-commit by running the following in the root atomate2 directory

pip install pre-commit
pre-commit install
pre-commit run --all

This should fix the failing linting.

Sorry for that, forgot to install it. Should be fixed now.

[utf]

Thanks!