Implement attribute setting for PeriodicSite to route through propert…

[cogsworth37]

…ies, ensuring persistence across serialization and structure sorting.

Summary

Major changes:

fixes materialsproject/pymatgen#2339

Checklist

• Google format doc strings added. Check with ruff.
• Type annotations included. Check with mypy.
• Tests added for new features/fixes.
• If applicable, new classes/functions/modules have duecredit @due.dcite decorators to reference relevant papers by DOI (example)

Tip: Install pre-commit hooks to auto-check types and linting before every commit:

pip install -U pre-commit
pre-commit install

[shyuep]

Thanks. There seems to be failing tests. Pls fix the test and linting errors.

[shyuep]

Automated PR review generated by Claude (Opus 4.7) on behalf of @shyuep. Based purely on the diff and current GitHub Actions state — no code was executed locally.

Nice fix for #2339 — routing arbitrary attribute writes through properties (with a DeprecationWarning) is the right tradeoff and the get_sorted_structure round-trip test is exactly the regression that this footgun warrants. The pickle/recursion guard for dunder attrs and _properties is correctly handled.

Blocking — CI is red:

1. Lint failures (must fix before merge):

   • src/pymatgen/core/sites.py:103-105 — three E501 Line too long (>120) errors. The warnings.warn(...) message is the culprit. Either break the f-string across lines or shorten the message.
   • tests/core/test_sites.py:246 — W293 Blank line contains whitespace. Just delete the trailing spaces on that blank line in the new docstring.

   These are auto-fixable via ruff check --fix and ruff format.

2. Test failures across all 4 matrix jobs (windows, macos, ubuntu 3.11 lowest-direct, ubuntu 3.14 highest). The visible portion of the log shows a few F markers before the runner was canceled by the matrix-fast-fail. The detailed assertion output isn't in the truncated logs, but at least one failure appeared in the tests/core/ block — please pull the full pytest log from the job and post the failing test names. It's quite possible the new __setattr__ is breaking some existing site-construction path that legitimately needed to set a non-property instance attribute (e.g. inside __init__ before _properties exists). Worth double-checking that super().__init__ paths still work cleanly.

Minor suggestions:

• The deprecation message embeds value!r directly in the warning text. For large value (e.g. a numpy array of magmoms), this could produce very long warning strings. Consider type(value).__name__ or truncating. Not blocking.
• The check attr in self.__dict__ happens after the property check, which is correct, but consider whether assignments to existing instance attrs should silently succeed (current behavior) or also warn — depends on intent.
• The new test is good; consider also asserting that the DeprecationWarning is actually emitted (pytest.warns(DeprecationWarning)) so future refactors can't silently drop it.

Will re-review once CI is green.

[shyuep]

Automated PR review generated by Claude (Opus 4.7) on behalf of @shyuep. Based purely on the diff and current GitHub Actions state — no code was executed locally. Follow-up to my prior review at f7df72a.

The new head 3d1511f is just a Merge branch 'main' into 2339-fix-footgun — there are no code changes since the prior review. Accordingly, the same blockers still hold and CI is still fully red.

Lint (still failing — exact same set):

• src/pymatgen/core/sites.py:103-105 — three E501 Line too long (>120) errors on the warnings.warn(...) message.
• src/pymatgen/core/sites.py:97:58 — PGH003 Use specific rule codes when ignoring type issues (the # type: ignore should be # type: ignore[<rulecode>]).
• tests/core/test_sites.py:246 — W293 Blank line contains whitespace.

All auto-fixable via uv run ruff check --fix . && uv run ruff format ..

Tests still failing: the matrix runs were cancelled by the lint fast-fail, but at least 3 visible F markers appeared in the tests/core/ block before cancellation on the 3.14 highest job. Please push the lint fix so the test matrix can complete cleanly, and post the failing test names so we can diagnose. As I noted before, the most likely cause is the new __setattr__ interfering with a path that legitimately sets a non-property instance attribute during construction.

Will re-review once green.

[shyuep]

Automated PR review generated by Claude on behalf of @shyuep. Based purely on the diff and current GitHub Actions state — no code was executed locally. Follow-up to my prior review at 3d1511f.

The new head fd7c35d ("Use object.setattr for setting attributes in Neighbor and PeriodicNeighbor classes") fixes exactly the failure mode I suspected last round: the new __setattr__ was intercepting nn_distance/index/image assignments inside Neighbor.__init__ / PeriodicNeighbor.__init__ (since these attrs are not properties and not yet in __dict__), triggering the deprecation warning and routing them into properties rather than as bare instance attrs. Routing those constructor writes through object.__setattr__ is the correct fix.

CI is now fully green — lint + all 4 test matrix jobs (windows/macOS/ubuntu-3.11-lowest/ubuntu-3.14-highest) pass. Lint nits from the prior review (E501, W293, PGH003) all resolved.

LGTM, ready to merge from my side. Two small non-blocking nits carried over from before, fine to address in a follow-up:

• sites.py:103-105 — the deprecation warning embeds value!r directly. For a numpy magmom array this could be very long; consider type(value).__name__ or truncating. Minor.
• The new test test_property_setattr_routes_to_properties doesn't actually assert that the DeprecationWarning fires (pytest.warns(DeprecationWarning)). Worth adding so a future refactor can't silently drop it.

One small architectural observation, also non-blocking: using object.__setattr__ in the Neighbor/PeriodicNeighbor constructors works, but it does mean any future Site subclass author has to remember to do the same dance for legitimate non-property instance attrs set during __init__. An alternative would be to seed __dict__["properties"] = {} first and rely on the attr in self.__dict__ branch — but that's a refactor preference, not a blocker.

[shyuep]

Thanks!