Structure.get_sorted_structure does not preserve magmom site property

[bocklund]

Describe the bug The Structure.get_sorted_structure method seems to clobber magmom attributes. As a consequence, all DictSet subclasses don't allow setting magnetic moments using site properties by default, since the default keyword argument sort_structure=True calls this method.

To Reproduce

from pymatgen.core import Structure
from pymatgen.io.vasp.sets import DictSet
struct = Structure.from_str("""Cr2
1.0
2.849623 0.000000 0.000000
0.000000 2.849623 0.000000
0.000000 0.000000 2.849623
Cr
2
direct
0.000000 0.000000 0.000000 Cr
0.500000 0.500000 0.500000 Cr""", fmt="POSCAR")

struct.sites[0].magmom = -5
struct.sites[1].magmom = 5

assert all(hasattr(site, "magmom") for site in struct.sites)  # passes
assert all(hasattr(site, "magmom") for site in struct.get_sorted_structure().sites)  # fails

from pymatgen.io.vasp.sets import MPStaticSet
assert all(hasattr(site, "magmom") for site in MPStaticSet(struct).structure.sites)  # fails
assert all(hasattr(site, "magmom") for site in MPStaticSet(struct, sort_structure=False).structure.sites)  # passes

Expected behavior

These two lines should produce (effectively) the same INCAR with a correctly set MAGMOM tag:

MPStaticSet(struct, user_incar_settings={"MAGMOM": False}, sort_structure=False).incar  # Works
MPStaticSet(struct, user_incar_settings={"MAGMOM": False}, sort_structure=True).incar  # Raises:
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-21-d89e9d171cfc> in <module>
----> 1 MPStaticSet(struct, user_incar_settings={"MAGMOM": False}, sort_structure=True).incar

~/.envs/dft-env/lib/python3.9/site-packages/pymatgen/io/vasp/sets.py in incar(self)
   1125         :return: Incar
   1126         """
-> 1127         parent_incar = super().incar
   1128         incar = Incar(self.prev_incar) if self.prev_incar is not None else Incar(parent_incar)
   1129

~/.envs/dft-env/lib/python3.9/site-packages/pymatgen/io/vasp/sets.py in incar(self)
    516                     elif hasattr(site.specie, "spin"):
    517                         mag.append(site.specie.spin)
--> 518                     elif str(site.specie) in v:
    519                         if site.specie.symbol == "Co" and v[str(site.specie)] <= 1.0:
    520                             warnings.warn(

TypeError: argument of type 'bool' is not iterable

[Andrew-S-Rosen]

Hi @bocklund,

I think this can be resolved by using the Structure.add_site_property() function, which is the preferred method of adding magnetic moments (and other site properties, like selective dynamics). I personally would recommend against trying to "manually" specify site properties, as they get stripped the moment you modify the structure.

Here is your patched example:

from pymatgen.core import Structure
from pymatgen.io.vasp.sets import DictSet
struct = Structure.from_str("""Cr2
1.0
2.849623 0.000000 0.000000
0.000000 2.849623 0.000000
0.000000 0.000000 2.849623
Cr
2
direct
0.000000 0.000000 0.000000 Cr
0.500000 0.500000 0.500000 Cr""", fmt="POSCAR")

# See here!
struct.add_site_property("magmom",[-5,5])

assert all(hasattr(site, "magmom") for site in struct.sites)  # passes
assert all(hasattr(site, "magmom") for site in struct.get_sorted_structure().sites)  # passes

from pymatgen.io.vasp.sets import MPStaticSet
assert all(hasattr(site, "magmom") for site in MPStaticSet(struct).structure.sites)  # passes
assert all(hasattr(site, "magmom") for site in MPStaticSet(struct, sort_structure=False).structure.sites)  # passes

This also confused me when I started out. I think that means we should make the documentation clearer.

[bocklund]

Thank you @arosen93, understood. add_site_property seems to be the solution. Your PR in #2342 is definitely helpful, but I didn't know that the add_site_property method existed and so the improved docs/examples may not have helped me in this case.

My thought process was that the code around the exception that I got in DictSet accessed magmom as an attribute, so the principle of least surprise would suggest that I can interact with site properties as I would attributes. It seems like there's some asymmetry in that Site.__getattr__ is overridden to access Site.properties, but there's not a symmetric Site.__setattr__ method that sets Site.properties.

[Andrew-S-Rosen]

I think this is a reasonable point. I would like to see the ability to do as you suggested because it is, arguably, the most intuitive route. I won't be able to make PR for this one, but perhaps you or someone else can improve upon that functionality.

[Andrew-S-Rosen]

@mkhorton -- I think this can be closed, but it's worth considering if a separate issue should be opened to address the pain point that @bocklund brought up.

[mkhorton]

Thanks for the reminder @arosen93, and thanks @bocklund for bringing this up -- it's true, I actually have not used the .magmom attribute myself and agree this looks confusing.

It seems like there's some asymmetry in that Site.getattr is overridden to access Site.properties, but there's not a symmetric Site.setattr method that sets Site.properties.

This seems like the fundamental cause. I would not close this issue until this has been addressed.