diff --git a/src/pymatgen/core/sites.py b/src/pymatgen/core/sites.py index 04b7ea94ff..519df38324 100644 --- a/src/pymatgen/core/sites.py +++ b/src/pymatgen/core/sites.py @@ -86,6 +86,29 @@ def __getattr__(self, attr: str) -> Any: return props[attr] raise AttributeError(f"{attr=} not found on {type(self).__name__}") + def __setattr__(self, attr: str, value: Any) -> None: + # Override setattr doesn't play nicely with pickle, + # so we can't use self._properties + if attr in {"properties", "_properties", "coords"} or attr.startswith("_"): + # Avoid infinite recursion when setting properties or _properties + super().__setattr__(attr, value) + elif isinstance(getattr(type(self), attr, None), property): + # If the attribute is a property, set it using the property setter + getattr(type(self), attr).fset(self, value) # type: ignore + elif attr in self.__dict__: + # If the attribute already exists on the instance, set it directly on the instance + super().__setattr__(attr, value) + else: + warnings.warn( + f"Setting attribute {attr} on {type(self).__name__} is deprecated and will be disallowed in a future version." + f" Please set this attribute on the 'properties' dict instead, e.g. site.properties['{attr}'] = {value!r}" + f" or use the structure.add_site_property('{attr}', {value!r}) method to add this property to all sites in a structure.", + DeprecationWarning, + stacklevel=2, + ) + props = self.__dict__.setdefault("properties", {}) + props[attr] = value + def __getitem__(self, el: Element) -> float: """Get the occupancy for element.""" return self.species[el] diff --git a/tests/core/test_sites.py b/tests/core/test_sites.py index 36307206eb..e7db3255d3 100644 --- a/tests/core/test_sites.py +++ b/tests/core/test_sites.py @@ -240,6 +240,35 @@ def test_setters(self): site.coords = [1.5, 3.25, 5] assert_allclose(site.frac_coords, [0.015, 0.0325, 0.05]) + def test_property_setattr_routes_to_properties(self): + """Setting an arbitrary attribute on a site should store it in site.properties, + not as a bare instance attribute, so it survives from_sites / get_sorted_structure.""" + + site = PeriodicSite("Fe", [0.25, 0.35, 0.45], self.lattice) + + # Assignment via attribute syntax must land in .properties + site.magmom = 5 + assert site.properties["magmom"] == 5, "magmom should be stored in site.properties, not as a bare attribute" + + # Round-trip through from_sites must preserve it + reconstructed = PeriodicSite.from_dict(site.as_dict()) + assert reconstructed.magmom == 5, "magmom lost after as_dict/from_dict round-trip" + + # Survival through get_sorted_structure is the real footgun + from pymatgen.core import Structure + + struct = Structure( + self.lattice, + ["Cr", "Fe"], + [[0, 0, 0], [0.5, 0.5, 0.5]], + ) + struct.sites[0].magmom = -5 + struct.sites[1].magmom = 5 + sorted_struct = struct.get_sorted_structure() + assert all( + "magmom" in site.properties for site in sorted_struct.sites + ), "magmom lost on one or more sites after get_sorted_structure" + def test_repr(self): assert repr(self.propertied_site) == "PeriodicSite: Fe2+ (2.5, 3.5, 4.5) [0.25, 0.35, 0.45]" assert repr(self.labeled_site) == "PeriodicSite: site label (Fe) (2.5, 3.5, 4.5) [0.25, 0.35, 0.45]"