Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/pymatgen/core/sites.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
29 changes: 29 additions & 0 deletions tests/core/test_sites.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]"
Expand Down