Structure.charge and as_dict() bug in recent versions of pmg

[fraricci]

Here what happens when I try to get a dict of a structure after applying a MagOrderingTransformation.

enumerator.transformations['afm'].apply_transformation(st).as_dict()

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-4-3d3739281aeb> in <module>
----> 1 enumerator.transformations['afm'].apply_transformation(st_p).as_dict()

~/python-venvs/main/lib64/python3.9/site-packages/pymatgen/core/structure.py in as_dict(self, verbosity, fmt, **kwargs)
   2206             "@module": self.__class__.__module__,
   2207             "@class": self.__class__.__name__,
-> 2208             "charge": self.charge,
   2209             "lattice": latt_dict,
   2210             "sites": [],

~/python-venvs/main/lib64/python3.9/site-packages/pymatgen/core/structure.py in charge(self)
    966         """
    967         if self._charge is None:
--> 968             return super().charge
    969         return self._charge
    970 

~/python-venvs/main/lib64/python3.9/site-packages/pymatgen/core/structure.py in charge(self)
    344         for site in self:
    345             for specie, amt in site.species.items():
--> 346                 charge += getattr(specie, "oxi_state", 0) * amt
    347         return charge
    348 

TypeError: unsupported operand type(s) for *: 'NoneType' and 'int'

I investigated where this comes from and found that, while the input structure has a _charge set to 0, the output structure has a _charge set to `None'. This lead to call the charge function in the as_dict(). And, as specie.oxi_state are None (I think this is general), the charge function is failing.

To note that in previous versions of pmg (at least 2022.0.4) the Structure.as_dict() had this line instead: "charge": self._charge This allows to avoid calling the charge function and not to crash for any values of _charge (int or None).

I see different problems here and I do not know what's the best approach:

1. simply go back to self._charge. I noticed that in the structure.py _charge is mostly used to get the charge
2. make MagOrderingTransformation set the _charge=0 as in the input structure. I haven't check if other transformers have the same issue
3. the function to compute the charge could be improved to handle oxi_state=None

Let me know what you think.

[fraricci]

I just noticed that @arosen93 changed that line 2208 "charge": self.charge. Could you comment on this?

[Andrew-S-Rosen]

Hi @fraricci --- thanks for the ping and apologies about any issues. It looks like the change was made in this PR based on this issue. I will look into this in more detail tomorrow --- I'm away from my computer at the moment. Hopefully that provides some insight though.

[fraricci]

I went down the rabbit hole a little more and I think the issue is in the MagOrderingTransformation at this line: https://github.com/materialsproject/pymatgen/blob/6345c201d1973afddb813342bd62e481daeee26a/pymatgen/transformations/advanced_transformations.py#L791

When it removes the dummy species, it explicitly define the oxi_state attr and set it to zero. This returns a structure where some sites have no oxi_state and some with it, making the charge func failing.

@mkhorton what you think?

[fraricci]

I proposed a fix here #2791, changing the Structure.charge(). I think it's the best way, since setting a Species.oxi_state to None seems something quite used, especially when dealing with spins. Also, Species.oxi_state can be None by its definition https://github.com/materialsproject/pymatgen/blob/6345c201d1973afddb813342bd62e481daeee26a/pymatgen/core/periodic_table.py#L1051

[janosh]

Fixed in #2791.