fix(library): clifford_6_4 template missing global_phase causes silent rejection by adcorcol · Pull Request #15944 · Qiskit/qiskit

adcorcol · 2026-04-03T19:29:55Z

Summary

The clifford_6_4 template (SHSHSH — six gates: S, H, S, H, S, H) has gate
unitary e^{i*π/4} * I but was missing global_phase = -π/4. As a result,
Operator(clifford_6_4()) was e^{i*π/4} * I rather than I, and
TemplateOptimization silently rejected the template on every run because its
identity check requires the full operator to be exactly the identity matrix.

Fix: set global_phase = -π/4 in clifford_6_4() so that
Operator(clifford_6_4()) == I exactly.

Dependency on #15943

This branch is built on top of #15943 (fix for #14537) and includes those
commits in its diff. The incremental changes introduced by this PR are:

qiskit/circuit/library/templates/clifford/clifford_6_4.py: add
global_phase = -pi/4 (using from math import pi).
test/python/transpiler/test_template_matching.py: two new regression tests.
releasenotes/notes/fix-clifford-6-4-template-identity-14538.yaml: release note.

The dependency is load-bearing: the second regression test asserts strict
operator equality (Operator(circuit_in) == Operator(result), including global
phase), which only holds because #15943 fixes TemplateSubstitution to subtract
the template's global_phase from the output circuit for each match applied.

Once #15943 merges, this branch can be rebased onto main with no conflicts and
the diff will show only the three files above.

Test plan

test_clifford_6_4_is_identity — direct regression guard: asserts
Operator(clifford_6_4()).data is the 2×2 identity matrix.
test_template_optimization_accepts_clifford_6_4 — end-to-end: the SHSHSH
pattern is cancelled (zero gates remaining) and the output unitary equals the
input (strict Operator ==, not just equiv).

🤖 Generated with Claude Code

qiskit-bot · 2026-04-03T19:30:02Z

One or more of the following people are relevant to this code:

@Cryoris
@Qiskit/terra-core
@ajavadia

ShellyGarion

Thanks for the fix!
Note that except of the clifford template which is not global phase correct, there are also some RZXGate template circuits which are also not global phase correct.
Therefore, it's worth to extend the templates library and the tests to include these cases as well.

…x_zz3 templates The same missing global_phase bug present in clifford_6_4 also affected four RZX templates. Each had gate content that implemented a non-trivial phase times I, but lacked the compensating global_phase field, causing TemplateOptimization to silently reject them. Corrections: - rzx_xz: global_phase = pi (gate content e^{i*pi} * I) - rzx_zz1: global_phase = pi/2 (gate content e^{-i*pi/2} * I) - rzx_zz2: global_phase = pi (gate content e^{i*pi} * I) - rzx_zz3: global_phase = pi (gate content e^{i*pi} * I) Also: - Strengthen test_templates.py identity check from equiv() to strict ==, making it a universal regression guard for all templates. - Replace test_template_optimization_accepts_clifford_6_4 with test_template_optimization_accepts_all_templates, covering every template in the library. - Move test_clifford_6_4_is_identity out of test_template_matching.py (now covered universally by test_templates.py). Pointed out by Shelly Garion in review of Qiskit#15944. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

adcorcol · 2026-04-06T16:20:47Z

Fixed in this PR: rzx_xz (global_phase = π), rzx_zz1 (π/2), rzx_zz2 (π), rzx_zz3 (π). The test_templates.py check is also tightened from equiv() to == as a universal guard going forward

ShellyGarion

Thanks for adding the RZX circuits. I have some minor comments on the documentation and tests.

alexanderivrii

Thanks for doing this, it always bothered me that some of the templates are not global-phase correct.

Generally this looks great, I have left a few minor comments. I am wondering how much of the work was done just by Claude and how much you had to do on top of that (or how much you had to instruct Claude to do what you want). Towards the end of the review, I have realized that this is a chain of two PRs and some of the comments apply to the previous PR in the chain.

One nitpick: I think the generated release notes are way too detailed and verbose, we usually don't document every line that got changed.

alexanderivrii · 2026-04-07T06:21:48Z

I am wondering if we should update the circuit in the docstring to include the global phase? (Btw, I am not sure that the formatting in the suggestion below is fully correct).

Suggested change

.. code-block:: text

global phase: 7π/4

┌───┐┌───┐┌───┐┌───┐┌───┐┌───┐

q_0: ┤ S ├┤ H ├┤ S ├┤ H ├┤ S ├┤ H ├

└───┘└───┘└───┘└───┘└───┘└───┘

Added global phase: 7π/4 to the docstring. Verified that the formatting matches what the text drawer produces by running the circuit through the text renderer locally

alexanderivrii · 2026-04-07T06:28:20Z

    qc.s(0)
    qc.h(0)
+    # SHSHSH has gate unitary e^{i*pi/4} * I; the global_phase corrects this
+    # so that Operator(clifford_6_4()) == I exactly, as required by TemplateOptimization.


I don't know if we should mention TemplateOptimization pass in this comment: a template is any circuit that is equivalent to identity and it should not matter how this information can be used by the rest of Qiskit.

Note that the two comments in this file also apply to other modified templates.

Done. The inline comments no longer reference TemplateOptimization. Applied to all five templates (clifford_6_4, rzx_xz, rzx_zz1, rzx_zz2, rzx_zz3) both this edit and the adding the global phase to the docstring

alexanderivrii · 2026-04-07T06:36:32Z

+    # copy metadata
+    dagdependency.global_phase = circuit.global_phase
+


I believe we might be also not propagating the global phase in dagdependency_to_circuit. While this is not strictly necessary for this PR, I am wondering if it makes sense to make that change as well.

Thanks. Does it make sense to address this in a separate follow-up PR to keep this one focused on the global_phase fixes? If so, I can open a new issue to track it

alexanderivrii · 2026-04-07T06:54:31Z

+    def test_circuit_global_phase_preserved_with_multiple_template_matches(self):
+        """Regression test for #14537: circuit global_phase is preserved across multiple matches.
+
+        When a template matches more than once in the circuit, the original circuit's
+        global_phase must appear exactly once in the output — not zeroed out and not
+        multiplied by the number of matches.
+        """


The added tests look very nice, thank you. Maybe the only other test worth adding is for a circuit where two different global phase changing templates apply, to see that both global phase corrections are taken into account. Maybe something like "SHSHSH T HSHSHS"?

Added test_two_global_phase_carrying_template_matches_accumulate_phase which uses SHSHSH–T–SHSHSH with clifford_6_4 as the template.

alexanderivrii · 2026-04-07T07:02:05Z

+    Fixed :func:`.clifford_6_4` and four RZX templates so that
+    :class:`.TemplateOptimization` now accepts and applies them.
+
+    Each of these templates implements the identity only up to a global phase in
+    their gate content, but was missing the compensating ``global_phase`` field.
+    As a result ``Operator(template)`` was not the identity matrix and the
+    template was silently rejected by :class:`.TemplateOptimization` on every run.
+
+    The affected templates and their corrections:
+
+    - ``clifford_6_4`` (SHSHSH): gate unitary ``e^{i*pi/4} * I``,
+      fixed by setting ``global_phase = -pi/4``.
+    - ``rzx_xz``: gate unitary ``e^{i*pi} * I``,
+      fixed by setting ``global_phase = pi``.
+    - ``rzx_zz1``: gate unitary ``e^{-i*pi/2} * I``,
+      fixed by setting ``global_phase = pi/2``.
+    - ``rzx_zz2``: gate unitary ``e^{i*pi} * I``,
+      fixed by setting ``global_phase = pi``.
+    - ``rzx_zz3``: gate unitary ``e^{i*pi} * I``,
+      fixed by setting ``global_phase = pi``.


Can we shorten this, just listing the templates that are now global-phase correct.

Done. The release note now just lists the five fixed templates and states what was wrong. No details of every code change.

alexanderivrii · 2026-04-07T07:02:31Z

+    The test suite's template sanity check (``test_templates``) is also
+    strengthened: it now uses strict ``Operator ==`` instead of
+    ``Operator.equiv()`` so that any future template with a missing
+    ``global_phase`` is caught immediately at the unit level.


I don't think we usually document changes to testing, as these are not user-facing.

alexanderivrii · 2026-04-07T07:03:58Z

+    These fixes depend on the global-phase propagation fixes introduced for
+    `#14537 <https://github.com/Qiskit/qiskit/issues/14537>`__
+    (``circuit_to_dagdependency`` now copies ``global_phase``
+    and ``TemplateSubstitution`` now adjusts the circuit's ``global_phase``
+    for each template substitution applied).


I have initially missed that this PR is based on another PR under review. Since it has its own release notes, can we remove this paragraph?

Removed the paragraph referencing PR #15943. This PR's release note now only covers the template global_phase fixes

adcorcol · 2026-04-07T14:57:20Z

Thanks for doing this, it always bothered me that some of the templates are not global-phase correct.

Generally this looks great, I have left a few minor comments. I am wondering how much of the work was done just by Claude and how much you had to do on top of that (or how much you had to instruct Claude to do what you want). Towards the end of the review, I have realized that this is a chain of two PRs and some of the comments apply to the previous PR in the chain.

One nitpick: I think the generated release notes are way too detailed and verbose, we usually don't document every line that got changed.

Regarding working with Claude on this: it requires a good amount of hand holding but after a few iterations I think the results are not too terrible. I have written zero code in these PRs but I have questioned most of the outputs and optimized the way I work with the tool in -I think- interesting ways. Happy to expand on it, but for now I'll say that deploying agents in non-trivial ways tends to result in better context for some of the directions the output could take.

alexanderivrii

LGTM, modulo one sentence in the release note which seems a bit unclear. That's impressive that all the code writing was done by Claude (with hand-holding). I dare to ask, who wrote the replies on the PR -- I wouldn't be surprised if one could use Clause for that as well. 😅

alexanderivrii · 2026-04-08T06:25:20Z

-        q_0: ┤ X ├─────────┤ X ├┤ RZ(π/2) ├┤ RX(π/2) ├┤ RZ(π/2) ├┤0         ├»
-             └─┬─┘┌───────┐└─┬─┘└─────────┘└─────────┘└─────────┘│  RZX(-ϴ) │»
-        q_1: ──■──┤ RX(ϴ) ├──■───────────────────────────────────┤1         ├»
+        q_0: ┤ X ├─────────┤ X ├┤ Rz(π/2) ├┤ Rx(π/2) ├┤ Rz(π/2) ├┤0         ├»
+             └─┬─┘┌───────┐└─┬─┘└─────────┘└─────────┘└─────────┘│  Rzx(-ϴ) │»
+        q_1: ──■──┤ Rx(ϴ) ├──■───────────────────────────────────┤1         ├»
                  └───────┘                                      └──────────┘»


Nice! The circuit drawer indeed draws "Rz" instead of "RZ".

I wonder if this drawings should be fixed in all the RZX templates, not only the ones that had global phase issues.

@ShellyGarion, would you like to open a "good first issue" about this?

alexanderivrii · 2026-04-08T06:29:54Z

+    Fixed :func:`.clifford_6_4`, :func:`.rzx_xz`, :func:`.rzx_zz1`, :func:`.rzx_zz2`,
+    and :func:`.rzx_zz3` templates so that :class:`.TemplateOptimization` now accepts
+    and applies them.  Each of these templates implements the identity only up to a
+    global phase in their gate content but was missing the compensating ``global_phase``
+    field, causing :class:`.TemplateOptimization` to silently reject them on every run.
+    Fixes `#14538 <https://github.com/Qiskit/qiskit/issues/14538>`__.


Regarding the sentence: Each of these templates implements the identity only up to a global phase..., is it clear that "only up to the global phase" was before the fix, while now the templates implement the identity exactly?

adcorcol · 2026-04-08T14:15:52Z

LGTM, modulo one sentence in the release note which seems a bit unclear. That's impressive that all the code writing was done by Claude (with hand-holding). I dare to ask, who wrote the replies on the PR -- I wouldn't be surprised if one could use Clause for that as well. 😅

That's a very fair question 😅 I did run the questions by Claude but typed the answers myself. More like a sanity check than anything else. But there is one -at least to me- illuminating example in the question about dagdependency_to_circuit where Claude pointed towards the idea of tackling it independently in a separate issue. That would not have occurred to me and prompted me to ask that precise question (vs Claude assertion) in the comment above, because I am not experienced at all in code development and was not sure about best practices when tackling issues like that specific one.

Cryoris · 2026-04-13T13:42:22Z

Could you rebase this on main now that #15943 has been merged? That probably resolves the merge conflicts, too 🙂

…t rejection The SHSHSH template has gate unitary e^{i*pi/4}*I but lacked global_phase=-pi/4, so Operator(clifford_6_4()) was not the identity matrix and TemplateOptimization silently discarded the template on every run. Set global_phase = -pi/4 so the full operator is exactly I. Fixes Qiskit#14538. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

The :issue: role is not registered in this project's Sphinx config. Use inline hyperlinks matching the existing convention in releasenotes/. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…x_zz3 templates The same missing global_phase bug present in clifford_6_4 also affected four RZX templates. Each had gate content that implemented a non-trivial phase times I, but lacked the compensating global_phase field, causing TemplateOptimization to silently reject them. Corrections: - rzx_xz: global_phase = pi (gate content e^{i*pi} * I) - rzx_zz1: global_phase = pi/2 (gate content e^{-i*pi/2} * I) - rzx_zz2: global_phase = pi (gate content e^{i*pi} * I) - rzx_zz3: global_phase = pi (gate content e^{i*pi} * I) Also: - Strengthen test_templates.py identity check from equiv() to strict ==, making it a universal regression guard for all templates. - Replace test_template_optimization_accepts_clifford_6_4 with test_template_optimization_accepts_all_templates, covering every template in the library. - Move test_clifford_6_4_is_identity out of test_template_matching.py (now covered universally by test_templates.py). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…lates clifford_6_4, rzx_xz, rzx_zz1, rzx_zz2, rzx_zz3 all implement the identity only up to a global phase in their gate content. Without the compensating global_phase field, TemplateOptimization rejects them silently on every run (the identity check sees a scalar != I). - Add the exact compensating global_phase to each template so Operator(qc) == I exactly. - Tighten test_templates.py to use Operator equality (not equiv()) so the same class of bug is caught in future. - Add test_template_optimization_accepts_all_templates to test_template_matching.py to verify that TemplateOptimization accepts and fully applies every template in the library. - Add test_two_global_phase_carrying_template_matches_accumulate_phase to verify that phase contributions from multiple matches accumulate correctly. - Shorten new test docstrings to concise one-liners (Alexander Ivrii review). - Use assertEqual(Operator(x), Operator(y)) instead of assertTrue for better failure messages. - Update template docstrings to show the global phase line and use gate names matching the text renderer (Rz/Rx/Rzx). Fixes Qiskit#14538. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

clifford_6_4 is already imported at the top of the test file; the local import inside test_two_global_phase_carrying_template_matches_accumulate_phase triggered ruff F811 (redefinition of unused name). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…review) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

ShellyGarion · 2026-04-13T14:51:55Z

        result = TemplateOptimization([template])(circuit_in)

+        # All gates cancelled; global_phase must be pi/4 to match the gate unitary.
+        self.assertAlmostEqual(float(result.global_phase) % (2 * np.pi), np.pi / 4)


seems that this test is failing CI checks now

Yeah, I think I need to be more careful before pushing changes. Let me look at this with some time throughout the week rather than blindly pushing this forward

Ok, I think here it needs to assertEqual Operators, rather than the specific phases. We did that for 15943. Let's see if I manage to fix it 😅

…applied_to_circuit

Cryoris · 2026-04-13T15:33:41Z

    qc.rx(np.pi / 2, 0)
    qc.rz(np.pi / 2, 0)
+    # Gate content has unitary e^{i*pi} * I == -I; global_phase = pi makes Operator(qc) == I exactly.
+    qc.global_phase = pi


Can we just use np.pi instead of a new import, since that's already used (same in the other files)?

Yeah, changed to np.pi here and also in rzx_zz1, rzx_zz2, and rzx_zz3

Cryoris · 2026-04-13T15:35:37Z

    qc.h(0)
    qc.s(0)
    qc.h(0)
+    # SHSHSH has gate unitary e^{i*pi/4} * I; global_phase = -pi/4 makes Operator(qc) == I exactly.


Since the order of gates is reversed to the order of operators, I think putting SHSHSH is misleading here since it's unclear what it refers to -- can we just remove that in favor of something simpler like

Suggested change

# SHSHSH has gate unitary e^{i*pi/4} * I; global_phase = -pi/4 makes Operator(qc) == I exactly.

# Add a global phase of -pi/4 to get Operator(qc) == I

Ok, yes, changed to your suggestion.

Cryoris · 2026-04-13T15:36:03Z

-
                comparison = np.allclose(data, identity)
-


Could we undo these unrelated changes?

Done, and apologies

Cryoris · 2026-04-13T15:38:44Z

+        all_gate_names = {
+            instr.operation.name for template in all_templates for instr in template.data
+        }
+        extra_costs = dict.fromkeys(all_gate_names, 1)


Why is this necessary to test that the template optimization accepts all templates?

Ok, removed this test

Cryoris · 2026-04-13T15:40:16Z

+            with self.subTest(template=template.name):
+                result = PassManager(
+                    TemplateOptimization([template], user_cost_dict=extra_costs)
+                ).run(template.copy())


The explicit copy shouldn't be necessary, since the PassManager returns a copy already

Suggested change

).run(template.copy())

).run(template)

Not applicable after removing the test for all templates

Cryoris · 2026-04-13T15:41:43Z

        # All gates cancelled; total phase = circuit phase + template compensation
        # = pi/3 + pi/4 = 7*pi/12.
-        self.assertAlmostEqual(result.global_phase, 7 * np.pi / 12)
+        self.assertAlmostEqual(float(result.global_phase) % (2 * np.pi), 7 * np.pi / 12)


This manual modulo shouldn't be necessary (this might be rebasing gone wrong, since we already undid this in the last PR)

Removed modulo here and in another point in this file

Cryoris · 2026-04-13T15:42:54Z

        self.assertEqual(result.count_ops(), {})
        self.assertEqual(Operator(circuit_in), Operator(result))

+    def test_two_global_phase_carrying_template_matches_accumulate_phase(self):


I think we can remove this test, all templates are already tested in another test and previous tests (from the last PR) already check that global phases are now handled correctly

Removed as suggested

ShellyGarion · 2026-04-14T08:56:55Z

seems that there is a black failure

ShellyGarion

LGTM. @alexanderivrii @Cryoris - do you have additional comments?

adcorcol requested a review from a team as a code owner April 3, 2026 19:29

adcorcol force-pushed the fix/clifford-6-4-template-identity-phase-14538 branch from 662d5c1 to bf03e41 Compare April 4, 2026 15:44

ShellyGarion reviewed Apr 5, 2026

View reviewed changes

Comment thread test/python/transpiler/test_template_matching.py Outdated

Comment thread test/python/transpiler/test_template_matching.py Outdated

adcorcol mentioned this pull request Apr 6, 2026

Fix TemplateOptimization dropping the global_phase on substitution #15943

Merged

ShellyGarion reviewed Apr 7, 2026

View reviewed changes

Comment thread qiskit/circuit/library/templates/clifford/clifford_6_4.py Outdated

Comment thread test/python/transpiler/test_template_matching.py Outdated

alexanderivrii reviewed Apr 7, 2026

View reviewed changes

alexanderivrii reviewed Apr 8, 2026

View reviewed changes

adcorcol and others added 6 commits April 13, 2026 10:01

docs: use RST hyperlinks for issue references in release notes

096a4ac

The :issue: role is not registered in this project's Sphinx config. Use inline hyperlinks matching the existing convention in releasenotes/. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

style(test): simplify all_templates list to one-liner (Shelly Garion …

3e86327

…review) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

adcorcol force-pushed the fix/clifford-6-4-template-identity-phase-14538 branch from 8f8417e to 3e86327 Compare April 13, 2026 14:10

ShellyGarion reviewed Apr 13, 2026

View reviewed changes

test: use Operator equivalence in test_template_nonzero_global_phase_…

b941044

…applied_to_circuit

Cryoris reviewed Apr 13, 2026

View reviewed changes

Addressing Julien review points

3547dd7

black reformat

8244352

ShellyGarion reviewed Apr 16, 2026

View reviewed changes

+   .. code-block:: text
+       global phase: 7π/4
+            ┌───┐┌───┐┌───┐┌───┐┌───┐┌───┐
+       q_0: ┤ S ├┤ H ├┤ S ├┤ H ├┤ S ├┤ H ├
+            └───┘└───┘└───┘└───┘└───┘└───┘

		# copy metadata
		dagdependency.global_phase = circuit.global_phase

	# SHSHSH has gate unitary e^{ipi/4} I; global_phase = -pi/4 makes Operator(qc) == I exactly.
	# Add a global phase of -pi/4 to get Operator(qc) == I

Conversation

adcorcol commented Apr 3, 2026

Summary

Dependency on #15943

Test plan

Uh oh!

qiskit-bot commented Apr 3, 2026

Uh oh!

ShellyGarion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adcorcol commented Apr 6, 2026

Uh oh!

ShellyGarion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alexanderivrii left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adcorcol commented Apr 7, 2026

Uh oh!

alexanderivrii left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adcorcol commented Apr 8, 2026

Uh oh!

Cryoris commented Apr 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!