Update cif.py

[LezheGao]

Resubmit of materialsproject/pymatgen#3071
Fixed materialsproject/pymatgen#3065

[shyuep]

Thanks. Pls add a unittest for this bug fix.

[shyuep]

Automated PR review generated by Claude (on behalf of @shyuep)

Fix is correct in principle — species properties (e.g. spin) should not leak into _atom_site_type_symbol (fixes materialsproject/pymatgen#3065). Two suggestions:

1. Avoid copy.deepcopy(sp) + mutation of the private _properties attr. Constructing a clean symbol is cheaper and doesn't touch internals, e.g. str(Species(sp.symbol, sp.oxi_state)) for Species (falling back to str(sp) for Element/DummySpecies). Deepcopy per (site × species) is measurable overhead when writing large structures.
2. Please add a regression test round-tripping a structure with spin-decorated species (see original issue) so this doesn't regress again.

CI is green (5/5).

[shyuep]

Automated PR review generated by Claude on behalf of @shyuep.

Re-review after new commits. The fix (rebuilding a clean Species/Element so CIF _atom_site_type_symbol doesn't carry spin/property decorations; resubmit of materialsproject#3071, fixes materialsproject#3065) is reasonable. Remaining items:

1. import copy is added but never used — remove it.
2. Single-quoted strings will fail ruff format — use double quotes.
3. The clean-species logic is duplicated in both branches; factor it into a small local helper.
4. Please add a regression test round-tripping a structure with spin-decorated species (the #3065 case).
5. CI has not run on this branch — needs a workflow approval after the above.

[shyuep]

Automated PR review generated by Claude on behalf of @shyuep. Re-review after the 2026-08-14 commits; static analysis only.

Thanks — items 1–3 from the last round are addressed (unused copy import gone, double quotes, logic factored into _clean_species). Three things still outstanding:

1. Regression for DummySpecies (new concern). DummySpecies.__init__ defaults oxidation_state=0, so sp.oxi_state is not None is True and the helper calls Species("X", 0), which raises since X is not an element. Any structure containing a dummy/vacancy species will now fail to write. Guard with isinstance(sp, DummySpecies) (already imported at cif.py:22) or rebuild via type(sp)(sp.symbol, sp.oxi_state).

2. Lint will fail. The blank line after return get_el_sp(sp.symbol) contains trailing whitespace (ruff W293), and there is no blank line between the __init__ docstring and the nested def. Please run ruff check src/pymatgen/io/cif.py && ruff format src/pymatgen/io/cif.py.

3. Still no test (item 4 from last time). Please add a round-trip covering materialsproject/pymatgen#3065 — a spin-decorated Species should write a clean _atom_site_type_symbol — plus one case with DummySpecies once (1) is fixed.

Also consider hoisting _clean_species to a module-level private function rather than redefining it per CifWriter.__init__ call. CI has still not run on this branch.

[shyuep]

Automated PR review generated by Claude on behalf of @shyuep. Re-review after the 2026-08-15 commits; static analysis only.

Both blockers from the last round are fixed: DummySpecies is guarded before the Species(...) reconstruction, and tests now exist. Remaining items, roughly in order of importance:

1. Formatting will fail lint. There is only one blank line between str2float and def _clean_species (cif.py:1565); ruff format requires two before a module-level def. Please run ruff check src/pymatgen/io/cif.py && ruff format src/pymatgen/io/cif.py.
2. Unrelated change — please restore the # type: ignore[reportPossiblyUnboundVariable] on the spg_analyzer.get_symmetrized_structure() line. spg_analyzer genuinely is conditionally bound there, so dropping the suppression will likely fail the type check for reasons unrelated to this fix.
3. _clean_species has no type hints, e.g. def _clean_species(sp: Element | Species | DummySpecies) -> Element | Species | DummySpecies:. Since both call sites only use str(clean_sp), having the helper return str directly would be simpler still.
4. test_write_dummy_species is close to a tautology. assert "X" in cif_str passes off the data_X1 block name alone, so it would still pass if the type symbol were wrong. Assert the actual _atom_site_type_symbol row, as test_write_spin_species effectively does.
5. Suggest dropping the __author__ edit — contributor attribution in this repo goes through git history and CHANGES.md, and the module __author__ list is reserved for original authorship.
6. Add a CHANGES.md entry (fixes materialsproject/pymatgen#3065; resubmit of materialsproject/pymatgen#3071).

CI has still not run on this branch — no checks reported on patch-1 across all 11 commits. A workflow approval is needed before merge; nothing above has been verified by an actual test run. The 11 Update cif.py commits should also be squashed on merge.

[shyuep]

Automated PR review generated by Claude on behalf of @shyuep.

Re-review of 13b1abd/2f8e2e4. The core fix is right: writing str(Species(symbol, oxi_state)) instead of str(sp) keeps Fe3+ in _atom_site_type_symbol rather than leaking ,spin=5.0 into the CIF. Three blockers:

1. test_write_dummy_species looks incorrect. _clean_species returns DummySpecies untouched, and DummySpecies("X") defaults to oxidation_state=0, so str(...) is "X0+", not "X". The assertion block.data["_atom_site_type_symbol"] == ["X"] should therefore fail. Please confirm locally — CI has never run on this branch (checks are pending awaiting first-time-contributor approval), so nothing has verified it.
2. Stray unrelated formatting change in cif.py: equivalent_sites # type: ignore[...] lost one of its two leading spaces. This repo runs ruff with select = ["ALL"], so E261 and ruff format --check will both fail. Please revert that hunk.
3. No CHANGES.md entry. Output of CifWriter changes for spin-carrying Species, which downstream string comparisons may depend on — worth a changelog line.

Minor: for a plain Element, get_el_sp(sp.symbol) is a needless round-trip; return sp is equivalent and cheaper. A short docstring note that spin is intentionally dropped (it is emitted separately in the magmom loop) would also help.

[Sanftperlig]

I looked at your _clean_species function and I think you can keep your general structure, but improve it a little bit to be cleaner

[shyuep]

Automated PR review generated by Claude (posted on behalf of @shyuep — not a human review; please treat as advisory).

Re-reviewing after the new commit. CI is red on all four test jobs, and the failure is this PR's own new test:

tests/io/test_cif.py::TestCifWriter::test_write_dummy_species
E   AssertionError: assert ['X0+'] == ['X']

(1 failed, 2780 passed — same single failure on ubuntu-3.11, ubuntu-3.14, macos-3.13 and windows-3.13, so it is deterministic, not flaky. lint passes.)

Root cause: DummySpecies("X") defaults to oxi_state = 0, not None, so str(sp) is "X0+". _clean_species returns DummySpecies unchanged via an early return, so the plain-dummy case never gets normalized. This contradicts the CHANGES.md entry added in this PR, which claims "plain DummySpecies output as X". Either the code or the claim needs to change:

if isinstance(sp, DummySpecies):
    return DummySpecies(sp.symbol) if not sp.oxi_state else sp

would make the code match the stated behaviour and the test. Note not sp.oxi_state (rather than is None) is what catches the 0 default — worth a comment so it does not get "cleaned up" later.

Other comments:

• hasattr(sp, "oxi_state") is redundant. By that line sp is neither Element nor DummySpecies, so it is a Species, which always has oxi_state. if sp.oxi_state is not None is clearer and lets the type checker help.
• The get_el_sp(sp.symbol) fallback (Species with oxi_state=None) round-trips through string parsing to get back an Element. Element(sp.symbol) is more direct and avoids get_el_sp's ambiguity for symbols that also parse as dummies.
• Stripping spin from _atom_site_type_symbol is the correct call — Fe3+,spin=5.0 is not a valid CIF type symbol — and keeping the magmom loop (covered by test_write_spin_species) means no information is lost. Good.
• CHANGES.md wording: "and resolves two failing tests" reads as if the PR fixes pre-existing failures; if the two tests are the ones added here, drop that clause.
• Consider adding a round-trip assertion (CifParser on the written string recovers the same species) for the spin case — currently test_write_spin_species only does substring checks, and "Fe3+" in cif_str would also pass if the spin suffix were still present.

Please push a fix for the failing test; happy to re-review after CI is green.

[Sanftperlig]

I think this is more or less the best you can do if you want to return a non-str object

If you return a str instead, you can do a one-liner (assuming I am not missing anything):

return str(sp).partition(",spin=")[0].replace("X0+", "X")

This always returns the pre-comma part (if there is a stated spin) and the dummy X0+ is forced to be X.
I believe this would also be faster (if I remember correctly isinstance is rather slow).

(Note: If you allow non-X dummies, you have to separate out the dummy case)

[Sanftperlig]

That is the correct implementation of my conversion line (with sp as str).
You could add a longer docstring, but I think it is fine (I also am not a maintainer though).

[shyuep]

🤖 Automated PR review generated by Claude (posted via @shyuep's account; static analysis of the diff + CI only, no code executed)

Good catch on the bug — str(sp) leaking ,spin= and X0+ into _atom_site_type_symbol produces CIFs that don't round-trip. A few things before merge:

1. _atom_type_symbol block is still unfixed. In the same __init__:

symbol_to_oxi_num = {str(el): float(el.oxi_state or 0) for el in sorted(comp.elements)}
blocks["_atom_type_symbol"] = list(symbol_to_oxi_num)

This still emits Fe3+,spin=5 / X0+ while _atom_site_type_symbol now emits Fe3+ / X. The two loops must agree or parsers that cross-reference _atom_type_symbol will mismatch. Apply _clean_species there too.

2. .replace("X0+", "X") is fragile. It only special-cases the symbol X, so DummySpecies("Xa") still yields Xa0+, and it silently discards a legitimate zero oxidation state for X only (Species("Fe", 0) keeps Fe0+). Prefer explicit logic, e.g. return sp.symbol when isinstance(sp, DummySpecies) and not sp.oxi_state.

3. Silent spin loss. With write_magmoms=False, spin is now dropped with no record anywhere in the file. That's better than an invalid symbol, but worth a docstring note on CifWriter (or a warnings.warn) so users know spin needs write_magmoms=True to survive.

4. test_write_spin_species assertion is too weak. assert "Fe3+" in cif_str also passes on the old buggy output Fe3+,spin=5, so the test doesn't guard the regression. Assert on the parsed value instead:

assert CifBlock.from_str(cif_str).data["_atom_site_type_symbol"] == ["Fe3+"]

A CifParser(...).parse_structures() round-trip check (species and magmom recovered) would be the strongest guard here.

5. Nit: the trailing comment in test_write_dummy_species ("No exception raised during write is already handled by the test") is a no-op — drop it.

CI: gh pr checks reports no checks reported on patch-1, so nothing has actually been validated yet. A maintainer needs to approve workflow runs for this first-time contributor branch before this can be assessed for merge.

[shyuep]

🤖 Automated PR review generated by Claude (posted on behalf of @shyuep)

Thanks for resubmitting this. The intent (strip spin from species strings in CIF output) is right, but a few issues need fixing before this can merge:

1. Dead/overriding assignment: in CifWriter.__init__, the new blocks["_atom_type_oxidation_number"] = list(symbol_to_oxi_num.values()) is immediately overwritten by the leftover old line blocks["_atom_type_oxidation_number"] = symbol_to_oxi_num.values(). Delete the old line.
2. _clean_species breaks on float oxidation states: Species.oxi_state is a float (and can be non-integral, e.g. Fe2.5+), so f"{sym}{oxi:+d}" raises ValueError. It also emits Fe+3 rather than the CIF-conventional Fe3+ that your own test asserts — test_write_spin_species will fail as written. Suggest reusing the oxi-state formatting logic from Species.__str__ (symbol + magnitude + sign) instead of reimplementing.
3. Style: only one blank line between _clean_species and class CifWriter — ruff (E302) requires two.
4. CI has not run on this branch; please push a fix so checks trigger.

[shyuep]

Automated PR review generated by Claude (scheduled routine, posted on behalf of @shyuep)

Thanks for resubmitting this — the goal (strip spin, keep oxi state in CIF species strings) is right, but CI is red (lint + 4 test jobs) and the diff has a few concrete bugs:

1. f"{sym}{oxi:+d}" produces Fe+3, but CIF convention — and your own test expectation ["Fe3+"] — is magnitude-then-sign (Fe3+). This is almost certainly the test failure. Suggest reusing Species.to_pretty_string()-style formatting.
2. :+d raises for non-integer oxi states (e.g. Fe2.67+) and oxi can be None (getattr(sp, "oxi_state", 0) returns None for Species with unset oxi state; None == 0 is False, so it falls through to the format call and crashes). The old code used el.oxi_state or 0.
3. blocks["_atom_type_oxidation_number"] is assigned twice — the second (leftover) line overwrites the list with dict.values(). Remove it.
4. symbol_to_oxi_num keyed on cleaned symbols can silently collide where distinct species clean to the same string (previously keyed on full str(sp)).
5. Lint failure: looks like a missing second blank line between _clean_species and class CifWriter. Please run ruff check / ruff format locally.

Happy to re-review once CI is green.

[shyuep]

Automated PR review generated by Claude (scheduled routine, posted on behalf of @shyuep)

Re-review after latest commits — all five issues from the previous automated review are now addressed: magnitude-then-sign formatting (Fe3+), None/non-integer oxi states handled in _clean_species, the duplicate _atom_type_oxidation_number assignment is gone, the symbol-collision concern is moot (oxi state is encoded in the key), and the blank-line lint issue is fixed. The added tests for spin-carrying Species and DummySpecies are good.

Two minor remaining points:

1. float(getattr(el, "oxi_state", 0)) raises TypeError when oxi_state is None, which silently drops the entire _atom_type_oxidation_number loop via the except fallback. The old code (el.oxi_state or 0) wrote 0.0 instead. Suggest float(getattr(el, "oxi_state", None) or 0).
2. '+'/'-' single-quoted strings in _clean_species will likely fail ruff format (double quotes enforced).

CI has not run on this branch (no checks reported — likely awaiting maintainer approval). Looks close to mergeable once the above are fixed and CI is green.