Skip to content

[breaking] Support mixed-supercell orderings in HeisenbergMapper via a shared parent-cell sublattice definition - #4700

Open
Luguza wants to merge 15 commits into
materialsproject:mainfrom
Luguza:luguza/heisenberg-mapper-mixed-supercells-4667
Open

[breaking] Support mixed-supercell orderings in HeisenbergMapper via a shared parent-cell sublattice definition#4700
Luguza wants to merge 15 commits into
materialsproject:mainfrom
Luguza:luguza/heisenberg-mapper-mixed-supercells-4667

Conversation

@Luguza

@Luguza Luguza commented Aug 11, 2026

Copy link
Copy Markdown

Summary

Refactors HeisenbergMapper so magnetic sublattices are defined once on a common parent cell, and every DFT-relaxed ordering is mapped onto it by structure matching rather than by direct index comparison. This decouples exchange-parameter fitting from any single ordering's cell size/shape, so orderings that were relaxed in different-sized supercells of the same parent can now be combined in one fit.

Closes #4667.

Motivation

The old implementation identified "unique sites" from the geometry of ordered_structures[0] and assumed every other ordering shared that exact indexing/cell. Orderings enumerated by a MagneticOrderingsWF-style workflow frequently don't: different orderings can relax into different (but commensurate) supercells of the same parent, and the mapper had no way to reconcile them. This branch introduces an explicit parent cell as the single source of truth for sublattice identity, with each ordering's sites matched onto it via StructureMatcher(attempt_supercell=True).

Key changes

  1. New MagneticOrdering / ParentOrdering / RelaxedOrdering classes (heisenberg.py) - each ordering now owns its magnetic-only structure, its NN graph, and its parent-sublattice labels. ParentOrdering derives sublattices from the parent's Wyckoff orbits (nonmagnetic ions kept in for correct site symmetry); RelaxedOrdering maps onto the parent via get_s2_like_s1 and raises a clear ValueError if it isn't a supercell of the parent.
  2. HeisenbergMapper(..., parent=None, ...) - new optional parent argument. If omitted, the parent is inferred as the primitive cell of the lowest-energy ordering (documented caveat: only safe if that ordering hasn't broken the paramagnetic symmetry).
  3. SublatticeMinimumDistanceNN - new NN strategy that takes the nearest shell per sublattice pair instead of one global nearest-neighbor window, so intra-sublattice bonds longer than the shortest inter-sublattice bond no longer silently drop out of the graph.
  4. Interaction labeling by nearest-shell-of-a-pair, not fixed ±tol windows.
  5. Least-squares fit over all supplied orderings (get_exchange) replacing the old square-system solve - surplus orderings now average out DFT-energy noise instead of being discarded; added an ill-conditioned/rank-deficient warning and an RMS residual (meV/magnetic ion) as a fit-quality metric.
  6. threshold_nonmag handling so induced moments on nominally nonmagnetic ions don't get miscounted as magnetic sites and change site counts/graph topology between orderings.
  7. Removed dependency from the core fit path; estimate_exchange and get_mft_temperature are now @deprecated (deadline 2027-08-01) in favor of get_exchange's shell-resolved J_ij and an external Monte Carlo solver, respectively.
  8. HeisenbergModel.from_dict fixed to handle unique_site_ids serialized as lists, and ex_mat serialization corrected (per PR HeisenbergModel.as_dict()/from_dict() round-trip broken: ex_mat serialized as a dict but parsed with literal_eval #4664).
  9. Units/docstring corrections throughout (J_ij in meV/μ_B² on raw moments vs. the meV-normalized-spin convention used by get_interaction_graph/VAMPIRE/TB2J).

Testing

All tests pass locally, including new coverage added on this branch:

TestHeisenbergMapperKnownHamiltonian - exchange recovery, RMS residual, degenerate-ordering dropping, Wyckoff-orbit sublattices, ill-conditioned-fit warning, shell counting per sublattice pair, interaction-graph consistency, incompatible (non-supercell) ordering rejection
TestHeisenbergMeanFieldTemperature - single/multi-sublattice MFT, exchange sign vs. ground state
TestHeisenbergMapperFullCellSymmetry - nonmagnetic ions splitting sublattices, symmetry-equivalent sites sharing one
TestHeisenbergMapperZeroMomentIon - quenched-moment species staying on the magnetic lattice

Luca Frey added 13 commits June 15, 2026 18:33
…parent once and with the nonmagnetic ions present. Orderings are mapped onto parent to determine what magnetic sublattice each site belongs to.
…itioned fit warning, correct units in docstrings
…ultiplying with the absolute magmoms at both ends of the edge
…bug and fix deprecation warnings. Remove TestHeisenbergMapper and move relevant tests from it to TestHeisenbergMapperHamiltonian

@shyuep shyuep left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Substantial, well-motivated refactor (parent-cell sublattice definition + StructureMatcher mapping + least-squares fit) with unusually thorough docstrings and new test coverage. Main comments:

  1. Attribution: don't comment out the original __author__ block (ncfrey). Either keep both authors or drop the dunder metadata entirely — git history carries attribution.
  2. raise SystemExit(...) was carried over from the old code — please use ValueError instead; SystemExit kills interactive sessions and notebooks.
  3. This is a significant API break for HeisenbergMapper (attributes like sgraphs, unique_site_ids, wyckoff_ids are gone; estimate_exchange/get_mft_temperature deprecated). A short migration note in the PR description / docs would help downstream users.
  4. CI has not run on this branch (first-time contributor — needs a maintainer to approve workflows). Please rebase on master and a maintainer should trigger CI before merge.
  5. Default-parent inference from the lowest-energy ordering is a documented footgun; consider emitting a UserWarning when parent=None so users can't miss it.

Nice work overall — the shell-midpoint interaction labeling and pooled magnetic species are clear improvements.

@Luguza

Luguza commented Aug 21, 2026

Copy link
Copy Markdown
Author

@shyuep @mkhorton As per the discussion in materialsproject/atomate2#1528, should I restore the vampire caller into pymatgen, because it is written to be specific to this workflow and relevant to local execution?

@Luguza
Luguza marked this pull request as ready for review August 21, 2026 15:59
@Luguza
Luguza requested a review from mkhorton as a code owner August 21, 2026 15:59
@Luguza
Luguza requested a review from shyuep August 21, 2026 15:59
@shyuep

shyuep commented Aug 21, 2026

Copy link
Copy Markdown
Member

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

Substantial and well-motivated refactor — defining sublattices once on a parent cell and mapping orderings via StructureMatcher(attempt_supercell=True) is the right architecture for mixed-supercell fits, and the migration guide in the module docstring is excellent. Points to address:

  1. Breaking changes without deprecation shims. Constructor arg order changes (parent in third position), get_exchange now returns a tuple, and sgraphs/unique_site_ids/wyckoff_ids/ordered_structures are removed outright. Consider deprecated property aliases for one release cycle where cheap, and mark the PR title [breaking] so the compatibility log picks it up.
  2. pymatgen-core submodule pointer bump rides along in the diff — drop it unless this PR actually requires it.
  3. Attribution: __author__/__maintainer__ replace @ncfrey entirely; please append rather than overwrite the original authorship.
  4. SublatticeMinimumDistanceNN is a nice generalization; the cached nn_graph guard raising before sublattice_ids are set is good defensive design.
  5. No CI results available yet (checks likely awaiting approval) — review based on code analysis only.

@Luguza Luguza changed the title Support mixed-supercell orderings in HeisenbergMapper via a shared parent-cell sublattice definition [Breaking] Support mixed-supercell orderings in HeisenbergMapper via a shared parent-cell sublattice definition Aug 23, 2026
@Luguza Luguza changed the title [Breaking] Support mixed-supercell orderings in HeisenbergMapper via a shared parent-cell sublattice definition [breaking] Support mixed-supercell orderings in HeisenbergMapper via a shared parent-cell sublattice definition Aug 23, 2026
@Luguza

Luguza commented Aug 23, 2026

Copy link
Copy Markdown
Author

Agreed on the title, I'll mark it [breaking] so it shows up in the compatibility log.

On the deprecation shims, I'd rather not add them here, and it's not only about cost. Taking them one at a time:

sgraphs nn_graphs would be an actively harmful alias. The old graphs were built on the full ordered structures, with every ion as a node; the new ones are built on the magnetic-only structure. The node sets differ, so any survivingsgraph.structure[i]or index-based loop would keep running and silently address a different site. Today that code fails loudly with an AttributeError on the first line that touches sgraphs. An alias would trade that for wrong numbers.

unique_site_ids sublattice_ids isn't an alias at all. The old attribute was a dict[tuple[int, ...], int] mapping tuples of equivalent supercell indices to an id; the new one is a list[list[int]] giving the parent-sublattice label of each magnetic site in each ordering. Bridging them means reconstructing the per-supercell orbit map — which is precisely the single-supercell assumption this PR exists to remove. wyckoff_idssublattice_wyckoff_symbols does happen to keep its dict[int, str] shape, but the ids underneath now index parent orbits rather than supercell orbits, so a bare alias would hand back keys that don't mean what the caller thinks.

get_exchange is breaking regardless of the return type. The {"<J>": ...} fallback for under-determined systems is gone and now raises ValueError, and the solve became a least-squares fit over all orderings rather than an exact inverse — so numbers move even for callers who never touch residual. Keeping the dict return would hide the signature change without making any of that safe.

There's also one break no shim can cover: energies keeps its name but now holds energy per magnetic ion instead of total energy. That's a silent numeric change in an attribute that still exists, which to me is the argument that a partial soft landing here is worse than a clean one — it lets callers get most of the way through a migration while quietly wrong.

Given that, I think the honest version is a hard break with a good migration path rather than a half-compatible surface. The module docstring already carries a migration guide covering every renamed and removed attribute, the constructor order, the get_exchange change, and HeisenbergScreener — worth a look if it didn't come through in the diff view. Happy to expand it, add a CHANGES.md entry, or move it somewhere more discoverable if you'd prefer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HeisenbergMapper silently returns wrong exchange parameters when orderings live in different-sized supercells

2 participants