Reuse first termination slabs in CoherentInterfaceBuilder._find_matches

[Sanftperlig]

Currently, CoherentInterfaceBuilder._find_matches always uses shift 0. As discussed in #4651 and materialsproject/pymatgen-core#105 this will lead to some primitivizations differing from the nonzero shifts in other functions.
I believe that to be the source of issues such as #4651 (and thus it closes #4651), but I do not have a test case, given the one in the issue works fine already (I do believe that this will cause errors in other cases where primitivization is shift-dependent though).
Additionally, passing the slabs to find_matches saves creating the SlabGenerator and generating the two slabs.

Note that this can change the returned interfaces and ZSL matches! This is the case if primitivization failed before (returning a non-primitivized slab) - now it should return the primitivized slab and thus the matches based on that.

Checklist

• Google format doc strings added. Check with ruff.
• Type annotations included. Check with mypy.
• Tests added for new features/fixes.

[shyuep]

Automated PR review generated by Claude (posted on behalf of @shyuep by a scheduled routine; static analysis of the diff + CI only — advisory, not a human review).

The diagnosis is right: _find_matches built its own SlabGenerator and took get_slab(shift=0), while _find_terminations uses get_slabs(...) at physically determined shifts. With primitive=True the Niggli/primitive reduction is shift-dependent, so ZSL matching could be done on a lattice that no constructed interface ever uses. Reusing a real termination slab is the correct direction, and dropping the duplicate generators is a free win.

Three things before this can go in:

1. film_slabs[0] is still only one termination. If primitivization is genuinely shift-dependent, slabs at other shifts can have different reduced lattices, so the matches remain inconsistent for terminations 2..N — the bug is narrowed, not removed. Preferred fixes, in order: (a) assert all slabs share the same lattice (np.allclose on the matrices) and raise/warn if not, so the silent inconsistency becomes loud; or (b) match on a non-primitivized generator so the lattice is shift-independent by construction. At minimum, add a comment stating the assumption that all terminations share a reduced lattice.

2. film_slabs[0] / sub_slabs[0] can IndexError. get_slabs(filter_out_sym_slabs=...) is not guaranteed non-empty; the old get_slab(shift=0) always returned a slab. Guard and raise a clear error naming the miller index.

3. No test. Understood that the #4651 reproducer already passes, but a regression guard is cheap and does not need a shift-dependent case: assert that the lattice _find_matches consumes is exactly next(iter(...)) of the termination slabs (e.g. via film_slabs[0].lattice.matrix), plus a characterization test on a standard pair (Si/GaAs) showing zsl_matches is unchanged. That pins the invariant this PR establishes.

Smaller points:

• __init__ now runs _find_terminations() before _find_matches(). Order inversion means the "Substrate lattice vectors changed during ZSL match" ValueError surfaces only after the expensive get_slabs() calls, and self.zsl_matches is now set last. Both private, both fine — just worth a line in the release notes since _find_matches's signature changed and downstream subclasses overriding it will break.
• Docstring formatting: _find_terminations needs a blank line between the summary and Returns: for the Google convention, and the Returns entry should describe the tuple rather than name it first_pair (tuple[Slab, Slab]: film and substrate slabs of the first termination). Same for the Args: block, which reads fine otherwise.
• Nit: Slab is annotation-only — it belongs in the TYPE_CHECKING block rather than the runtime import.

CI: no checks have reported on shift-change and mergeable_state is blocked — workflows need maintainer approval for this fork. Please approve the run; the review above is static-analysis only, with no test evidence behind it.

[shyuep]

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

The diagnosis is right: matching against get_slab(shift=0) while get_interfaces uses shift-dependent primitivized slabs is inconsistent, and reusing the slabs already built in _find_terminations removes two redundant SlabGenerator constructions. Points to address:

1. film_slabs[0] is still an arbitrary choice. The fix replaces "shift 0" with "first termination", but if primitivization is shift-dependent then different terminations can yield different reduced lattices, and the ZSL match will still be inconsistent for every termination but the first. Either document this assumption explicitly in the docstring, or verify that all slabs share the same reduced lattice and raise/warn otherwise. Also, film_slabs[0] will IndexError if get_slabs returns empty, where the old code would not.
2. A test is feasible even without a reproducer. You cannot reproduce Singular film supercell transformation matrix when calling get_interfaces for CoherentInterfaceBuilder #4651, but you can lock in the new invariant: assert that the lattices in zsl_matches match those of the first-termination slabs from _find_terminations for an existing interface test case. That prevents silent regression back to shift=0.
3. Docstring will fail ruff/pydocstyle. _find_terminations needs a blank line between the summary and the Returns: section (D205), and the Google convention does not name the return variable — drop first_pair (tuple[Slab, Slab]) and just describe the tuple.
4. Minor readability: prefer film_slab, sub_slab = self._find_terminations() followed by self._find_matches(film_slab, sub_slab) over self._find_matches(*self._find_terminations()). _find_terminations now both mutates state and returns a value, which is worth a one-line comment.

No CI has run on this branch (workflows awaiting approval), so correctness against the existing interface tests is unverified.

[shyuep]

Thanks for fixing this issue. Can you add a unittest pls? Using a new example is fine. There is no need to rely on the existing examples.

[Sanftperlig]

I sadly don't have time to work on this more significantly right now, but I will see if I can create a small test - I'm not too familiar with the CoherentInterfaceBuilder compared to the SlabGenerator, so the test might not be too great.
I'm not familiar with semiconductors, so it would probably be a test around the calcite case where I know this issue is a thing.

To address the Claude comments: To my knowledge, get_slabs always returns slabs if valid terminations can be found. That should always be the case (unless bonds are defined, which they aren't in this case)? Thus it would be fine to use [0].
As far as I am aware any produced terminations result in the same slab lattice, thus checking the other indices shouldn't be necessary.

Sorry for the delay and thank you for running the CI.

[shyuep]

Automated PR review generated by Claude (posted on behalf of @shyuep; not a human review — please sanity-check before acting.)

Good catch on the root cause: hard-coding shift=0 in _find_matches while get_interfaces uses the termination-specific shifts is exactly the kind of inconsistency behind #4651, and reusing the slabs from _find_terminations also removes a duplicate SlabGenerator pass. Three things to fix before merge.

1. The new test will error, not pass. Interface.lattice is a Lattice, which does not define __getitem__:

assert np.isclose(inter.lattice[0, 0], 5, atol=1e-4)   # TypeError: 'Lattice' object is not subscriptable

Use inter.lattice.matrix[0, 0] / [1, 1]. Since no CI has run on this PR yet (zero status checks on 3ff6ad2a), this was not caught. While there, prefer the already-imported assert_allclose(inter.lattice.matrix[0, 0], 5, atol=1e-4) over assert np.isclose(...) for the file's style and better failure messages — that also drops the new import numpy as np.

2. film_slabs[0] / sub_slabs[0] is an unguarded, arbitrary choice.

• IndexError if get_slabs() returns an empty list. The old get_slab(shift=0) could not fail this way, so this is a new failure mode for edge-case Miller indices. Worth an explicit error message.
• More substantively: the ZSL matches are now computed from the first termination's primitivized cell but applied to every termination in get_interfaces. That is only sound if the in-plane lattice is termination-independent. If that invariant is assumed, please state it in the docstring; if it is not guaranteed, a cheap assert/raise comparing film_slabs[i].lattice.matrix[:2] across terminations would surface the violation instead of silently producing wrong matches. The current docstring ("A slab of the film structure, transformed as used in the other functions") glosses over this.

3. Behavior change is user-visible and undocumented. zsl_matches, and hence the strains and supercells returned by get_interfaces, can change for existing users whose structures primitivize shift-dependently. Please say so in the PR description and flag it for release notes — silently different interface geometries are hard to debug downstream (atomate2 uses this).

Minor: the hard-coded termination label "CaCO_Pmma_6" makes the test brittle against any change in label_termination or spglib version; consider selecting cib.terminations[0] instead. Also the checklist still shows "Tests added" unchecked although a test is present.

Nothing else concerning in the refactor — the __init__ reordering is safe since _find_terminations does not read zsl_matches, and the added type annotations are correct.

[shyuep]

Automated PR review generated by Claude (sanity-check before acting on it)

Nice, well-scoped fix. _find_matches previously rebuilt its own shift=0 slabs via a fresh SlabGenerator, which could primitivize differently than the slabs actually enumerated in _find_terminations, so the ZSL lattice-matching could run against a lattice inconsistent with the interface later returned by get_interfaces. Reusing film_slabs[0]/sub_slabs[0] from _find_terminations closes that gap, and test_shiftdependent_primitive (calcite, space group 167) is a good regression test that actually exercises the shift-dependent primitivization case rather than just re-asserting the old behavior.

Two things worth a look before merge:

• _find_terminations returns film_slabs[0], sub_slabs[0] unconditionally. If filter_out_sym_slabs (or ftol) ever yields an empty film_slabs/sub_slabs for a given miller index, this is now an IndexError in __init__ instead of the old independent get_slab(shift=0) call, which always succeeded. Worth a guard/clearer error if that's reachable in practice.
• No CI has run on this PR (no checks reported on the 'shift-change' branch) — it's a fork PR (Sanftperlig/pymatgen), so this is likely just pending workflow approval rather than a CI problem, but flagging since there's no green signal yet to weigh against the diff.

Diff itself (interfaces + reordered __init__) looks correct and minimal.

[shyuep]

Automated PR review generated by Claude (posted on behalf of @shyuep; not a human review — please sanity-check before acting.)

Solid fix, well-targeted at the root cause. _find_terminations already builds film_slabs/sub_slabs via get_slabs(); reusing film_slabs[0]/sub_slabs[0] in _find_matches instead of spinning up two more SlabGenerator/get_slab(shift=0) calls removes duplicated, inconsistent slab generation and directly addresses #4651 (shift-0 primitivization sometimes disagreeing with other shifts). The new np.allclose guard on matrix[:2] (in-plane a/b vectors only, ignoring the vacuum-dependent c-vector) is the right check, and turning a previously-silent wrong-lattice bug into a loud ValueError is the correct call for a scientific library — much better than quietly returning bad ZSL matches. itertools.product ordering confirms self.terminations[0] does correspond to (film_slabs[0], sub_slabs[0]), so the docstring's "first termination" claim checks out. The new test_shiftdependent_primitive calcite case is a good regression test and the PR body is upfront about the behavior change (interfaces/ZSL matches can shift for structures where shift-0 primitivization previously failed).

Two things worth a look before merge:

• No CI has run on this PR yet (zero status checks on the latest commit) — given the new ValueError guards are stricter than before, it'd be worth confirming the full analysis/interfaces suite (and anything downstream, e.g. pymatgen-core) still passes rather than newly raising on existing fixtures.
• Minor nit: the film/sub lattice-consistency blocks in _find_terminations are near-identical (empty-list check + allclose loop + ValueError); could be factored into a small helper to avoid the duplication, though not blocking.

[shyuep]

Automated PR review generated by Claude (on behalf of @shyuep). Sanity-check before acting.

Good: Sound fix for the shift-0 vs shifted-primitivization mismatch (#4651), and reusing the slabs from _find_terminations removes two redundant SlabGenerator builds. Docstrings and the new regression test (calcite, R-3c) are useful.

Issues:

• New hard failure in __init__. _check_equal_slab_lattices raises ValueError whenever slabs across shifts have different in-plane lattices. The PR description itself says primitivization can be shift-dependent, so structures that constructed fine before (using shift-0 only) will now raise at construction, even if the user only wants one termination. Consider warning instead of raising, or only validating the slabs actually used for _find_matches.
• Brittle test. The test comment says termination names "are allowed to change", yet it asserts cib.terminations[0] == ("CaCO_Pmma_6", "CaCO_Pmma_6"). Assert the lattice (the real invariant) and drop or relax the label check. There is also no test for the new ValueError paths (empty slabs, mismatched lattices).
• No CI has run on this branch (no checks reported on 'shift-change'), so nothing here is validated. Please push a commit or re-run workflows from the fork.
• Slab-parameter mismatch. _find_matches previously built slabs with min_slab_size=1, min_vacuum_size=3, center_slab=True, primitive=True, reorient_lattice=False. Please confirm _find_terminations uses the same settings; only lattice.matrix[:2] is compared, so any c-vector difference would pass the check silently. Verify ZSL only consumes the in-plane vectors.
• _find_matches now silently depends on _find_terminations being called first (signature change); fine for a private method, but note it in the docstring.

Nits: _check_equal_slab_lattices lacks -> None. Release note should flag that returned interfaces/ZSL matches can change (as the PR text says).

[Sanftperlig]

On the different comments:

New hard failure in init.

I have never encountered a case where one selected termination (by get_slabs, or more accurately SlabGenerator.gen_possible_terminations) has a lattice difference to another. That of course does not mean that that is impossible, but I wouldn't know where this would happen. It would of course be possible to switch to a warning, if that is preferable.

Brittle test.

I added the name check intentionally such that it fails if someone modifies the function more significantly. It's supposed to make them aware of a potential difference in the chosen termination - so that a fail due to differing lengths can be assigned to the correct source (different asserted termination instead of incorrect primitivization).

Slab-parameter mismatch.

It's the same SlabGenerator, just with different terminations. Also, as far as I am aware (based on the paper), ZSL matching is 2D based on the surface and has no third-vector component.

_find_matches now silently depends on _find_terminations being called first

I don't understand this issue. _find_matches doesn't need _find_terminations: You could use gen_possible_terminations, then generate the first slab pair via get_slab and use that in _find_matches. I did not do that, as we generate all terminated slabs already in _find_terminations, this way we save 2 slab generations. On the docstrings: _find_matches clearly indicates it wants two terminated slabs as arguments, while _find_terminations clearly states it returns two terminated slabs (and that you can use them for checking primitivization).

[shyuep]

Automated PR review generated by Claude (posted under @shyuep's account) — sanity-check before acting. Based on code and CI only; nothing was run.

Good: Sound idea: _find_matches hard-coded shift=0 while _find_terminations and get_interfaces use nonzero shifts, so primitivization can differ. Reusing the first termination slab pair removes the duplicated SlabGenerator setup, and the lattice-equality guard turns a silent assumption into an explicit check. Type hints/docstrings are clean.

Substantive

• No CI has run on this PR (zero status checks reported), so the green signal here is absent, not positive. Approve workflows before merging.
• New ValueError in __init__ is a potential regression: structures whose shifted slabs have differing in-plane lattices previously constructed fine (using shift 0) and now raise. Confirm this cannot trigger on the existing test structures or common inputs (e.g. with filter_out_sym_slabs=True, check that film_slabs/sub_slabs at the check site are the same set used for terminations).
• The error path (_check_equal_slab_lattices with diverging lattices, and the empty-slabs path) is untested.
• Call order inverted (_find_terminations before _find_matches) and _find_matches now takes required args; fine for a private method, but check for subclasses/external callers.

Nits

• test_shiftdependent_primitive pins cib.terminations[0] == ("CaCO_Pmma_6", "CaCO_Pmma_6"); brittle against label changes, and the test comment already says names may change. Assert on lattice only, or on termination count.
• Docstring says "Raises: ValueError: If no slabs are present"; the message for the empty case reuses "Could not generate Slabs", good, but doesn't mention which structure (film vs substrate) beyond miller/composition.