[Breaking] Deprecate Orbital.dx2 with Orbital.dx2_y2

[DanielYang59]

close materialsproject/pymatgen#4588

previously materialsproject/pymatgen#4590

Breaking Changes

Orbital.dx2 is renamed to Orbital.dx2_y2 (enum value 8 unchanged). To avoid breaking consumers of serialized data, this is a two-phase transition:

• Until 2027-08-17: Orbital.dx2 / Orbital["dx2"] still resolve (with a DeprecationWarning); CompleteCohp.as_dict() / CompleteDos.as_dict() still write the legacy dx2 key; readers accept both dx2 and dx2_y2.
• From 2027-08-17: canonical dx2_y2 is serialized and the legacy alias/read paths are removed.

Migration: use Orbital.dx2_y2 / "dx2_y2"; existing serialized data needs no conversion.

[shyuep]

🤖 Automated PR review generated by Claude, posted on behalf of @shyuep. PR is marked draft, so this is preliminary — based on static analysis of the diff; no code was executed locally.

Clean, well-scoped deprecation of Orbital.dx2 → Orbital.dx2_y2. CI is green on all platforms.

• ✅ Enum value 8 is preserved, so Orbital(8) and any index-based (de)serialization stay stable — the important backward-compat invariant.
• ✅ Legacy serialized data is handled in BandStructure.from_old_dict, CompleteCohp.from_dict, and CompleteDos.from_dict, each with a one-time DeprecationWarning (nice touch keeping the warning out of the inner loop) and matching pytest.warns coverage. IO parsers (cp2k/jdftx/vasp) now emit the canonical name.
• 🔹 _OrbitalMeta.__getattribute__ fires a string comparison on every class-level attribute access (Orbital.dxy, Orbital.s, …), not just the deprecated one. Since dx2 is no longer a real member, hooking __getattr__ on the metaclass instead would trigger only on the missing-attribute (deprecated) path and remove that per-access overhead. __getitem__ is already the correct minimal hook.
• 🔹 In cohp.py, canonical_orb = orb.replace("dx2", "dx2_y2") is guarded by "dx2_y2" not in orb, which is fine, but consider an exact-token replace to avoid surprises on any composite orbital label that merely contains the substring dx2.
• 🔹 Worth a changelog/release-note entry calling out that Orbital.dx2 now warns and will be removed, since this affects downstream code and serialized objects.

LGTM in direction; ready for full review once taken out of draft.

[shyuep]

Automated PR review generated by Claude (on behalf of @shyuep; re-review after new commits)

Overall this is a clean deprecation: legacy dx2 handling in from_dict/from_old_dict paths, one-time DeprecationWarnings, and good test coverage. Full CI matrix is green. A few points:

1. _OrbitalMeta.__getattribute__ overhead — this intercepts every class-level attribute access on Orbital, including hot parsing loops (Orbital.s, Orbital(kk), etc.). Since dx2 no longer exists as a member, defining __getattr__ on the metaclass instead would only fire on missing names and be zero-cost for everything else.
2. Serialization forward-compat (your open checkbox) — agreed this is the main risk: dicts emitted by as_dict() will now contain dx2_y2, which older pymatgen versions and downstream consumers (atomate2, emmet, lobsterpy) that key on "dx2" cannot read. Suggest either emitting the legacy key for one deprecation cycle or explicitly calling this out in the release notes as a schema change.
3. cohp.py key rewrite — orb.replace("dx2", "dx2_y2") guarded by "dx2_y2" in orb works, but a more targeted regex (or splitting the label on -) would be more robust against future label formats.
4. Pickle compat is fine since Orbital reduces by value and dx2_y2 keeps index 8.

Marked as draft, so treat this as preliminary feedback. Nice work on the exhaustive IO-path updates (cp2k, jdftx, vaspout.h5).

[shyuep]

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

Re-review of the new commits (rename + legacy-serialization compatibility). The deserialization shims in bandstructure.from_old_dict, CompleteCohp.from_dict and CompleteDos.from_dict look correct, with single-shot warnings — good.

1. _OrbitalMeta.__getattribute__ intercepts every class-attribute access on Orbital (including enum-internal machinery and hot loops over members). Since dx2 no longer exists as a member, a metaclass __getattr__ would fire only on the missing name and is essentially free for all other access. Recommend switching.
2. canonical_orb = orb if "dx2_y2" in orb else orb.replace("dx2", "dx2_y2") — fine for LOBSTER-style labels, but a brief comment on the expected key format would help.
3. Your own flag on as_dict() now emitting dx2_y2 is the real breaking change for downstream consumers. Suggest an explicit changelog/release-note entry, and consider keeping serialization output as-is for one release with only reads accepting both.

CI green (5/5). Otherwise LGTM.

[DanielYang59]

Let's mark this as draft for now, I still haven't got time to check potential downstream effect and at least we should implement #120 first...

[shyuep]

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

Re-review after the force-push (3067c49/711d5e1). The rename is complete and consistent across dos, plotter, cp2k, jdftx and vasp/outputs, and the read-side shims with one-shot DeprecationWarnings are correct. However, all three points from the previous review are still open:

1. _OrbitalMeta.__getattribute__ still intercepts every class-attribute access on Orbital, including enum internals and hot loops (Orbital.dxy, Orbital.dz2, ... in get_site_t2g_eg_resolved_dos). Since dx2 is no longer a member, a metaclass __getattr__ fires only on the missing name and costs nothing otherwise. Please switch.
2. canonical_orb = orb if "dx2_y2" in orb else orb.replace("dx2", "dx2_y2") still has no comment on the expected LOBSTER key format, and would also rewrite an unrelated substring dx2 in a composite label.
3. No CHANGES.md/COMPATIBILITY.md entry. as_dict() now emits dx2_y2, which is the actual breaking change for downstream consumers — see #128 for the new [breaking] convention.

CI green (5/5). Still marked draft, so no merge action taken.

[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 fully green (lint + all four test matrix jobs). The rename itself is correct — dx2 was always a misnomer for d_{x^2-y^2} — and the legacy-name handling in from_dict/from_old_dict for CompleteDos, CompleteCohp and BandStructure covers the read path well, with the warned_legacy_dx2 flag keeping the warning to one per deserialization.

On the open question in the PR description (downstream effects of as_dict()): the read path is handled, but the write path is the real break. After this PR as_dict() emits dx2_y2, so files written by new pymatgen cannot be read by older pymatgen or by any external consumer that keys on "dx2" (LOBSTER post-processing tooling, MP/emmet documents, atomate2 parsers). Two options:

• Conservative: keep writing "dx2" in as_dict() for one release cycle while accepting both on read. Zero forward-compat break, deprecation warning on write instead.
• Clean break: write "dx2_y2" as done here, but bump the document version / add an explicit note to CHANGES.md so downstream can gate on it.

I'd lean conservative given how much serialized COHP/DOS data exists in the wild, but either is defensible — it just needs to be a deliberate decision recorded in CHANGES.md, which this PR does not touch.

Specific code comments:

1. _OrbitalMeta.__getattribute__ should be __getattr__. __getattribute__ intercepts every class-level attribute access on Orbital, so it adds a Python-level function call plus a string comparison to hot paths like the orb in (Orbital.dxy, Orbital.dxz, Orbital.dyz) checks inside get_site_t2g_eg_resolved_dos and the projection loops. Since dx2 is no longer a member, __getattr__ on the metaclass is only reached on the miss and is free otherwise:

def __getattr__(cls, name):
    if name == "dx2":
        warnings.warn(...)
        return cls.dx2_y2
    raise AttributeError(name)

2. canonical_orb = orb if "dx2_y2" in orb else orb.replace("dx2", "dx2_y2") in cohp.py is fragile. These labels are composite (e.g. "2px-3dx2"), and a label containing both spellings — or one where dx2 appears twice — short-circuits on the guard and gets only partly renamed. Safer to split on the separator and map each token, or use a regex with a boundary: re.sub(r"dx2(?!_y2)", "dx2_y2", orb).

3. Orbital.dx2_y2.name is now "dx2_y2". Downstream code doing orbital.name == "dx2" or str(orbital) comparisons breaks silently — the Orbital.dx2 / Orbital["dx2"] aliases do not help there. Worth an explicit line in the CHANGES.md/migration note.

4. No deprecation timeline. DeprecationWarning with no target removal version tends to live forever. Suggest naming a release in the message (pymatgen has used @deprecated/deprecated decorators elsewhere for this).

5. Minor: EnumMeta is the legacy alias for EnumType (3.11+). Fine for the supported floor, but EnumType if/when 3.11 is dropped.

Still marked draft — the substance is in good shape; the blocker is deciding the as_dict() policy in point 0 and documenting the break.

[shyuep]

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

Rename coverage checks out: I swept every remaining dx2 occurrence under src/ and the leftovers are all unrelated (idx2 locals, and the JDFTx perturb-ion dx2 displacement tag in jdftxinfile_master_format.py). Legacy-name handling is also correctly scoped to BandStructure.from_old_dict only, since modern as_dict stores projections positionally rather than orbital-keyed. CI is green.

Two substantive comments:

1. Drop the DeprecationWarning from the as_dict() paths. CompleteDos.as_dict() and CompleteCohp.as_dict() now warn whenever dx2_y2 is present — i.e. for every user whose code is already fully correct. There is no action the caller can take to silence it, and it will fail any downstream suite running -W error::DeprecationWarning (a common pymatgen-adjacent config). The legacy-key emission is an internal compatibility decision; the # TODO(2027-08-17) comments already record it. Recommend keeping the TODOs and deleting both warnings.warn blocks. The read-path warnings (_OrbitalMeta, from_old_dict) are the right place for user-facing deprecation and should stay.

2. _OrbitalMeta.__getattr__ should delegate rather than hard-raise.

raise AttributeError(name)

On Python 3.11 (the project floor) EnumType.__getattr__ still participates in member and alias resolution, so unconditionally raising can shadow lookups the base metaclass would have served. Safer:

try:
    return super().__getattr__(name)  # type: ignore[misc]
except AttributeError:
    raise AttributeError(name) from None

Also worth noting hasattr(Orbital, "dx2") now emits the warning, so downstream feature-detection code will get noise — acceptable, but a stacklevel sanity check on that path would be nice.

Minor: the warnings.warn messages say "removed on 2027-08-17" / "on or after 2027-08-17" for the same event — worth making the two strings consistent.

Otherwise the approach (metaclass shim + __getitem__ shim + symmetric ser/deser) is sound and the added tests in test_core.py cover the deprecation surface well.

[shyuep]

Automated PR review generated by Claude (posted on behalf of @shyuep). Re-review after the latest commits (conservative serialization path + deprecation deadline).

• Two-phase design is sound: enum value 8 unchanged; _OrbitalMeta.__getattr__/__getitem__ alias dx2 → dx2_y2 with DeprecationWarning and an explicit 2027-08-17 removal date; serializers keep writing the legacy dx2 key for now so downstream consumers of serialized data are unaffected. TODO(2027-08-17) markers on every legacy path make phase 2 easy to execute.
• Legacy read paths look complete: CompleteCohp.from_dict (incl. mixed labels via re.sub(r"dx2(?!_y2)")), CompleteDos.from_dict, and BandStructure.from_old_dict (one-shot warning). Round-trip tests assert no spurious DeprecationWarnings on canonical data — nice.
• IO mappings (cp2k, jdftx, vaspout.h5, plotter) consistently updated.
• All CI checks green (lint + full test matrix).

LGTM.

[shyuep]

Automated PR review generated by Claude (scheduled routine, posted on behalf of @shyuep) — re-review after new commits since the 2026-07-19 automated review.

LGTM. The two-phase deprecation is well executed: enum rename with _OrbitalMeta alias emitting DeprecationWarning on both Orbital.dx2 and Orbital["dx2"], legacy dx2 keys still serialized until 2027-08-17 with clearly dated TODOs, and read paths accepting both spellings. Test coverage is thorough (round-trips under error-level warning filters, mixed-label handling, legacy JSON fixtures). CI is fully green.

Two minor notes, neither blocking: (1) code that compares orbital.name == "dx2" on an instance (rather than via the class alias) will now silently mismatch — the COMPATIBILITY entry from the PR body covers this, just make sure the migration note calls it out; (2) the AGENTS.md addition about machine-generated files is a nice touch and belongs in this PR given it documents the [breaking] workflow this PR itself uses.