-
Notifications
You must be signed in to change notification settings - Fork 868
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
Clearer handling of the MAGMOM flag in pymatgen.io.vasp.sets #2301
Conversation
The VASP default is 1.0, not 0.6. The 0.6 comes from a PMG default
NUPDOWN should be an integer, but the sum of MAGMOM entries need not be an integer. The user should be warned if this happens.
Test is failing because magmoms are on sites
Thanks @arosen93! For the PR itself, if WIP, please either put [WIP] in the title or you can explicitly "convert to draft" in the top-right. For the proposed changes, this looks like an important thing to clarify, and the PR looks good. I did also want to make sure you knew about the In terms of handling of magmom, we've ended up in a slightly awkward situation:
This is broader context not immediately relevant to this PR, but may be good to bear in mind if you're editing this code. (Edit: sorry, submitted this comment too soon and missed the second half) |
Thanks. |
Currently, the handling of the MAGMOM INCAR flag in
pymatgen.io.vasp.sets
could be made clearer. This became apparent to me when trying to modify MAGMOM in an atomate workflow, which did not at all do what I expected it to do (and led to some confusing errors). This PR does not change how magnetic moments are handled in Pymatgen. It just adds and clarifies some warning messages.An error is now raised if MAGMOM is not a dict, and the user is appropriately notified that this is an element-specific flag, not a site-specific flag like the typical VASP INCAR's MAGMOM argument.
The
constrain_total_magmom
flag, when set toTrue
, sums up the magnetic moments on each site and sets that as NUPDOWN. If the user specified a custom MAGMOM, then there is no guarantee the summed values are integers. I have therefore added a warning ifround(sum(magmoms)) != sum(magmoms)
.The warning about Co being initialized as low-spin could be made clearer. Currently, Pymatgen prints out this warning whenever MAGMOM has an entry for Co and there is no site-specific
magmom
orspin
. If one were to change the config to have high-spin Co or modify MAGMOM directly (e.g.{"MAGMOM": {"Co":5}}
), the warning would still appear despite the fact that it is no longer relevant. I have clarified the wording in the warning.The documentation says that if the symbol is not in the config file, VASP's default of 0.6 is used. The VASP default is 1.0; the Pymatgen default is 0.6. I've clarified the phrasing.
Of course, if the user specified the magnetic moments as site properties, much of this wouldn't be needed, but this subtlety surrounding how MAGMOM is treated wasn't initially clear to me.