Add whole motif flip strategy to MagneticStructureEnumerator

[Luguza]

Summary

MagneticStructureEnumerator with the ferrimagnetic_by_motif strategy could not produce orderings where one or more whole Wyckoff motifs are flipped antiparallel to the rest, with every motif staying internally ferromagnetic. The Néel ground state of a spinel ferrimagnet like Fe3O4 (4f up, 2c down) is one of these. The existing ferri_by_motif_{symbol} constraints make one motif internally AFM, and afm forces equal up/down counts, so neither one reaches these orderings.

This PR adds them as ferri_by_motif_{symbols}_flip orderings, e.g. ferri_by_motif_2c_flip or ferri_by_motif_b_c_flip.

Implementation

• Built directly, not enumerated. When every motif is internally FM, the magnetic cell is the crystallographic primitive cell, so picking which motifs to flip fixes the ordering. _add_whole_motif_flips builds these structures and appends them to ordered_structures, the same way the fm ordering is already handled. It does not call enumlib.
  • An earlier version of this PR used a global order parameter (num_motif_sites / num_mag_sites). That fixes only how many spins point up, not which sites carry them. On Fe3O4 it produced six orderings, and only one of them was the whole-motif flip. The Néel state survived only because it was the most symmetric, so truncate_by_symmetry / max_orderings could drop it on other structures. The direct construction replaces that approach, together with its order parameter == 0.5 guard, which also made the half-multiplicity flip unreachable when antiferromagnetic was not requested.
• All combinations, no inversion duplicates. The PR emits every non-empty subset of motifs to flip, with the first motif always held up. Flipping a set of motifs and flipping its complement differ only by a global spin inversion, so n motifs give 2**(n-1) - 1 orderings. With max_unique_sites=8 that is at most 127. All of them keep the full crystal symmetry, so truncate_by_symmetry does not remove them.
• Reproducible origins. wyckoff_symbols is now sorted by descending multiplicity, then by symbol, instead of iterating a set. Before this change, which of two equivalent orderings survived duplicate pruning (and so which origin label was recorded) could change with PYTHONHASHSEED.
• Adapted to the duplicate-pruning fix in Fix MagneticStructureEnumerator duplicate-ordering pruning never running #4702.

Tests

• Fe3O4 Néel input is recognized as ferri_by_motif_2c_flip.
• Each motif stays internally FM in the flipped structure. The existing ferri_by_motif_4f / ferri_by_motif_2c transformations are unchanged.
• Motifs of equal multiplicity (Ca3Co2O6, 2a/2b) give a single flip, not one per motif.
• Three motifs give exactly the three expected subsets and spin patterns.
• For 2–6 motifs: the output has 2**(n-1) - 1 orderings, and none is the spin inversion of another.
• The flip is emitted when only ferrimagnetic_by_motif is requested, without antiferromagnetic.
• No two surviving orderings on Fe3O4 are duplicates according to matches_ordering.

The TestMagneticStructureEnumeratorMotifFlip tests skip __init__ and do not need enumlib.

[shyuep]

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

Two distinct changes here:

1. Dedup fix (== 0 → > 0): correct — the old inverted conditional made duplicate pruning a no-op. This is a real bug fix worth fast-tracking; consider splitting it into its own PR so it isn't held up by the feature.
2. ferri_by_motif_{symbol}_flip: the global order parameter num_motif_sites / num_mag_sites reads correctly (whole-motif flip is reachable via the global up-fraction, and the spin-inversion degeneracy makes the sign choice irrelevant). Note when a motif is exactly half the magnetic sites, this duplicates the AFM constraint (op = 0.5) — harmless once dedup works, which reinforces point 1.

Blocking: no tests yet (acknowledged in your Todos) — both the dedup regression test and flip-strategy coverage (spinel Néel case) are needed before merge. CI has not run on this branch and will need approval.

[shyuep]

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

The ferri_by_motif_{symbol}_flip strategy is a sound addition — whole-motif flips (e.g. spinel Néel states) are indeed unreachable via the existing internally-AFM motif constraints, and expressing it as a single global order parameter to avoid _remove_dummy_species is well-reasoned. Skipping motifs at exactly half the magnetic sites (would duplicate the global AFM constraint) is correctly handled and tested; complementary flips (OP p vs 1−p) correctly rely on the dedup pruning, which the Fe3O4 test verifies.

Points to address: (1) the dedup one-liner here duplicates #4702, which is a fix-only PR with its own regression tests — suggest rebasing this PR to feature-only once that lands; (2) the pymatgen-core submodule pointer bump should be dropped from the diff unless intended; (3) the PR body TODOs claim tests are missing, but the diff includes them — please update the checklist. No CI results available yet; review based on code analysis only.

[Luguza]

@shyuep what should I do with the submodule bump, it was intended on my side?

[Luguza]

@shyuep Thank you for merging #4702, now let's tackle this PR. Can you approve the CI workflows?

[shyuep]

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

Clean, focused addition. The whole-motif flip is a real gap in the current ferrimagnetic_by_motif strategies (spinel Néel orderings being the obvious case), and expressing it as a single global order parameter to avoid the _remove_dummy_species issue is a sensible workaround. The num_motif_sites * 2 == num_mag_sites guard against duplicating the global AFM constraint is correct, and the Ca3Co2O6 test covers exactly that edge. Test coverage is thorough, including the complementary-flip pruning behavior.

Comments:

1. PR description is stale. The inverted-conditional dedup fix in _generate_ordered_structures is no longer in this diff — it landed via #4702, which merged yesterday. Please update the description/Todos and rebase; the stated merge blocker ("Ready for merge once #4702 lands") is now satisfied.
2. The pymatgen-core submodule pointer bump (1e86cf8 → 3affe46) appears incidental to this change — confirm it's intentional or drop it from the PR.
3. CI has not run on the latest commits ("no checks reported"). Needs a workflow approval/trigger before merge evaluation.

Once rebased with CI green, this looks mergeable.

[shyuep]

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

The ferri_by_motif_{symbol}_flip strategy is a genuine gap-fill (whole-motif Néel orderings like magnetite were unreachable), and the tests are strong — the Fe3O4 flip-recovery test, the complementary-flip pruning test, and the Ca3Co2O6 half-multiplicity skip all target the right edge cases.

Points to address:

1. Rebase on master and update the description. The inverted dedup conditional described in the body is not in this diff — it landed via Fix MagneticStructureEnumerator duplicate-ordering pruning never running #4702, which is now merged. The "Ready for merge once Fix MagneticStructureEnumerator duplicate-ordering pruning never running #4702 lands" gate is satisfied; rebasing will also let the pruning tests here run against the merged fix.
2. Revert the pymatgen-core submodule pointer bump (same accidental change as in [breaking] Support mixed-supercell orderings in HeisenbergMapper via a shared parent-cell sublattice definition #4700).
3. Half-multiplicity skip assumes the AFM constraint exists. The 0.5-order-parameter flip is skipped on the grounds that it duplicates the global afm constraint, but that constraint is only added when "antiferromagnetic" is in strategies. If a user requests only "ferrimagnetic_by_motif", the ordering is silently unreachable. Consider emitting the flip when "antiferromagnetic" not in self.strategies.
4. Housekeeping: the two unchecked "Todos" in the body appear to be done — please update.
5. CI has not run on this branch; will approve workflows after the rebase.

[Luguza]

@shyuep Addressed the outstanding review points:

1. Rebased cleanly onto main. The branch had accumulated redundant commits from repeated merges (a duplicate dedup fix and a duplicate AGENTS.md/pymatgen-core sync commit, both content-free once merged against current main) - dropped both. The branch is now 3 commits on top of main, and the pymatgen-core submodule pointer is untouched (matches main exactly, no diff).

2. Fixed the half-multiplicity edge case you flagged. The ferri_by_motif_{symbol}_flip skip for a motif that's exactly half the magnetic sites assumed the global afm constraint (op=0.5) was always present to make it a duplicate. If a caller requests only "ferrimagnetic_by_motif" without "antiferromagnetic", that constraint is never added, so the flip was silently unreachable. Now only skipped when "antiferromagnetic" is in self.strategies, with a new regression test (test_half_multiplicity_flip_kept_without_antiferromagnetic_strategy).

Could you approve the CI workflows on the latest commits?

[shyuep]

Automated PR review generated by Claude (posted on behalf of @shyuep by a scheduled routine; static analysis of the diff and GitHub state only, no code executed).

Re-review after the 2026-09-03 rebase. All four previous points are addressed: the branch is clean on main, the pymatgen-core submodule pointer is untouched, and the half-multiplicity skip is now gated on "antiferromagnetic" in self.strategies with a dedicated regression test. Nice.

Remaining items:

1. The constraint is not motif-scoped, but the label says it is. all_constraints[f"ferri_by_motif_{symbol}_flip"] is a bare float order parameter — no site_constraint_name, no species_constraints (the test asserts exactly this). So ferri_by_motif_4f_flip enumerates every 4-up/2-down arrangement of the Fe sublattice, not specifically the whole-motif flip. For Fe3O4 that happens to contain the Néel state, but any structure that survives gets ferri_by_motif_4f_flip written into ordered_structure_origins regardless of whether it is a motif flip. That is misleading provenance metadata. Either rename to something order-parameter-flavoured (ferri_op_2_3, in line with ferri_by_{sp}) or state the limitation in the docstring.

2. The naming is inverted for >2 motifs. op = num_motif_sites / num_mag_sites is the fraction left pointing up, so ferri_by_motif_A_flip describes "A up, everything else down" — i.e. flip the complement of A. With two motifs the two labels are each other's spin inversion so it does not matter; with three or more, ferri_by_motif_A_flip is not a flip of A.

3. Nondeterministic origin labels. wyckoff_symbols is a set, so for symbol in wyckoff_symbols iterates in an order that varies with PYTHONHASHSEED. Since the flip constraints are mutual spin inversions and matches_ordering treats those as equal, which label survives dedup changes between runs — test_complementary_motif_flips_are_pruned accepts either precisely because of this. Iterating sorted(wyckoff_symbols) (in both this loop and the existing ferri_by_motif loop above) makes ordered_structure_origins reproducible for a one-word change.

4. Duplicate constraints for equal-multiplicity motifs. Two motifs with the same site count produce identical order parameters under different keys — test_half_multiplicity_flip_kept_without_antiferromagnetic_strategy asserts both ferri_by_motif_2a_flip and ferri_by_motif_2b_flip are emitted for Ca3Co2O6, which runs enumlib twice on the same constraint. Deduplicating by order-parameter value would keep the coverage and halve the work.

5. The PR title still advertises the dedup fix, which landed separately in Fix MagneticStructureEnumerator duplicate-ordering pruning never running #4702 and is not in this diff. Please update.

6. CI still has not run on this branch (first-time-contributor workflow approval). Nothing here should be merged before it does.

None of 1–4 are blockers for correctness of the enumeration; 3 is the one I would fix before merge.

[Luguza]

Thanks — points 1–4 are all addressed, but working through (1) turned up
something that changes the implementation rather than just its labelling.

The constraint did not do what its name claimed. You flagged that
ferri_by_motif_4f_flip is a bare order parameter with no site_constraint_name.
That is worse than misleading provenance: because the order parameter fixes only
the global up:down count, the strategy enumerates every 4-up/2-down arrangement
of the Fe sublattice. On Fe3O4 it returns six orderings and only the first is a
whole-motif flip:

{'2c': {-1: 2}, '4f': {1: 4}}                    <- Neel state, motifs intact
{'1a': {1:1}, '1b': {1:1}, '2c': {-1:2, 1:2}}    <- 2c broken internally
{'2a': {1: 4, -1: 2}}                            <- one orbit, mixed spins
... 3 more

The Neel state survives here only because it is the most symmetric of the six, so
truncate_by_symmetry or max_orderings could discard it on a structure where it
is not. The gap from #4698 was therefore not really closed.

Whole-motif flips need no enumeration. With every motif internally FM the
magnetic cell is the crystallographic primitive cell, so choosing which motifs to
flip determines the ordering outright. _add_whole_motif_flips now constructs them
directly and appends to ordered_structures, the way the fm ordering already
does. Fe3O4 yields one ordering instead of six, guaranteed motif-intact, with no
enumlib call and a label that is genuinely motif-scoped.

The strategy now covers one or more motifs flipped together, i.e. every non-empty
subset, which matters for three-sublattice ferrimagnets. Inversion is handled
upfront by holding the first motif up, rather than relying on the duplicate pruning:
a flip and its complement are spin inversions, and exactly one of the pair contains
the first motif. Verified exhaustively for 2–6 motifs — 2**(n-1) - 1 orderings,
no generated pair an inversion of another, every inversion class covered once.

Two of your points dissolve rather than being fixed:

• (2) inverted naming for >2 motifs — the label names the motifs actually
  flipped, so it cannot be inverted.
• (4) duplicate constraints for equal-multiplicity motifs — gone with the
  order parameters. Ca3Co2O6's 2a and 2b now give a single flip ordering.

(3) is fixed as before: wyckoff_symbols is sorted by descending multiplicity,
so origin labels no longer depend on PYTHONHASHSEED.

One correction to #4698. The issue says the per-motif form (1 on the motif, 0 on
the rest) "trips _remove_dummy_species, which drops the flipped motif's dummy
species". That is not the mechanism. With those order parameters _add_dummy_species
returns an ordered structure, so apply_transformation takes the trivial-case
shortcut, skips enumlib, and fails in _remove_dummy_species with neighbors=[] —
get_neighbors(site, 0.05) finds nothing for a dummy sitting at exactly zero
distance from its parent site. Enumeration normally perturbs coordinates enough to
hide it. Sidestepped entirely here since nothing is enumerated, but it is a genuine
latent bug on that path and I am happy to open a separate issue.

(5) Title updated.

(6) Could you approve the CI workflows? Locally the magnetism suite passes,
including new coverage that each motif stays internally FM
({"4f": {5}, "2c": {-5}}) and that no generated flip is the spin inversion of
another, parametrised over 2–6 motifs.

[shyuep]

Automated PR review generated by Claude (posted on behalf of @shyuep by a scheduled routine; static analysis of the diff and GitHub state only, no code executed).

Re-review of acd24758. The rewrite is a real improvement over the order-parameter approach: building the flips directly removes the misleading provenance (points 1-2 of my last review), sorted(set(wyckoff) - {"n/a"}, key=lambda s: (-wyckoff.count(s), s)) fixes the PYTHONHASHSEED nondeterminism (point 3), holding the first motif up removes the inversion duplicates (point 4), and the half-multiplicity special case is gone entirely. The reasoning that each motif being internally FM makes the magnetic cell the crystallographic primitive cell — so no enumeration is needed — is right.

Five things on the new code:

1. 2**(n-1) - 1 orderings is unbounded growth. With max_unique_sites = 8, a structure with 8 magnetic Wyckoff motifs now emits 127 extra candidate structures from this one strategy, where the old code emitted 8 constraints. Each survives into ordered_structures as a DFT candidate. Realistic systems rarely hit 8 magnetic motifs, but nothing here caps it. Either bound num_flipped (most physically interesting cases are 1, and 2 for complex ferrimagnets) or expose a limit in transformation_kwargs. As written a single unlucky input turns a workflow into ~130 static calculations without warning.

2. _generate_transformations now mutates self.ordered_structures as a side effect. There is precedent (fm is appended there too), but the method name and its -> dict[str, MagOrderingTransformation] signature both say otherwise, and the tests had to be restructured around it. Worth at least a line in the method docstring saying it also emits orderings directly.

3. Heterogeneous site properties in ordered_structures. flipped_structure = structure.copy() inherits the "wyckoff" site property added at line ~829, while fm_structure explicitly strips magmom and the enumlib-derived structures carry neither. So the user-facing ordered_structures list now contains structures with different site-property keys depending on origin, which will bite anyone serializing or StructureMatcher-ing them uniformly. flipped_structure.remove_site_property("wyckoff") after the spin assignment (and having the test read wyckoff off structure instead) fixes it.

4. The origin label is not round-trippable for multi-motif flips. f"ferri_by_motif_{'_'.join(flipped)}_flip" gives ferri_by_motif_b_c_flip, which cannot be distinguished from a hypothetical single motif named b_c and is awkward to parse downstream. "+".join or "-".join would be unambiguous. Cosmetic, but these strings end up in ordered_structure_origins and in user workflows.

5. The sort key changes existing labels. Ordering motifs by descending multiplicity also changes which symbol ferri_by_motif_{symbol} gets credited with relative to main, so previously-recorded origins will not match new runs even for the pre-existing strategies. Given [breaking] Support mixed-supercell orderings in HeisenbergMapper via a shared parent-cell sublattice definition #4700 is explicitly marked [breaking] and this one is not, please call this out in the PR description.

Minor: wyckoff.count(symbol) inside the sort key is O(n) per comparison — collections.Counter(wyckoff) is cleaner and O(n) total.

Blocking, unchanged from last time: CI has still never run on this branch (no checks reported on 'luguza/magnetic-enumerator-whole-motif-flips'). Nothing here should merge before the first-time-contributor workflow approval goes through and the matrix is green. Of the five points above, 1 and 3 are the ones I would fix before merge.

[shyuep]

🤖 Automated PR review generated by Claude on behalf of @shyuep. Not a human review — treat as a first pass.

Good contribution. The gap is real — ferri_by_motif_{symbol} makes a motif internally AFM rather than flipping it as a block, so the Néel ground state of a spinel ferrimagnet was genuinely unreachable. Holding the first motif up to quotient out global spin inversion is the right trick, and the Fe₃O₄ / Ca₃Co₂O₆ tests are well chosen.

Design concern: side effects inside _generate_transformations

_add_whole_motif_flips appends to self.ordered_structures and self.ordered_structure_origins from inside a method whose contract is to return transformations. That creates an ordering dependency: if _generate_ordered_structures ever reassigns (rather than appends to) those lists, the flips vanish silently. Please either

• have _add_whole_motif_flips return (structures, origins) and let the caller merge them, or
• add an assertion/test that _generate_ordered_structures preserves pre-existing entries.

The existing test test_each_motif_stays_internally_ferromagnetic asserts ordered_structure_origins == ["fm", "ferri_by_motif_2c_flip"], which would catch a reassignment — but only on the object.__new__ path that bypasses __init__.

Combinatorial cost

2**(n-1) - 1 orderings, up to 127 at max_unique_sites=8. Each is a full Structure.copy() and — more importantly — each becomes a DFT calculation for anyone driving this from a workflow. There is no way to opt out short of dropping ferrimagnetic_by_motif entirely, which also loses the internally-AFM orderings. Suggest either a separate strategy name ("ferrimagnetic_by_motif_flip") so users can select independently, or a cap on num_flipped with the default at 1 (the physically common case) — the current all-subsets behaviour is a large silent increase in cost for existing users.

Smaller points

1. sorted(set(wyckoff) - {"n/a"}, key=lambda s: (-wyckoff.count(s), s)) is O(n²) via count inside the key. Use collections.Counter(wyckoff) — negligible at these sizes but free to fix. Determinism fix itself is a good catch and arguably worth its own note in the release notes, since it changes which origin gets recorded for existing users.
2. -abs(magmom) / abs(magmom) discards the sign of the input magmoms. Correct for generating a fresh ordering, but means an input structure's own ordering is not preserved in these entries. Worth one line in the docstring.
3. ordered_structure_origins gains a new format (ferri_by_motif_{a}_{b}_flip). Anything downstream parsing these strings (atomate2, emmet) will see an unfamiliar shape — flag as a soft API change.
4. Tests use object.__new__(MagneticStructureEnumerator) plus manual attribute assignment. It works and the comment explains why, but it hard-codes the attribute set of __init__; adding a required attribute later breaks these tests with a confusing AttributeError. A @classmethod test helper on the class, or a pytest fixture, would age better.
5. The docstring claim "All keep the full crystal symmetry, so none are removed by truncate_by_symmetry" is a strong invariant. Is it tested for truncate_by_symmetry=True? The two tests that exercise it pass truncate_by_symmetry=False.

CI hasn't reported (0 checks) — please push or rebase so the suite runs before merge.

[shyuep]

Thanks! I have personally reviewed the addition and it is great!