Fix `TemplateOptimization` dropping the `global_phase` on substitution by adcorcol · Pull Request #15943 · Qiskit/qiskit

adcorcol · 2026-04-03T18:20:34Z

Summary

TemplateOptimization was silently dropping or miscalculating global_phase in three places:

circuit_to_dagdependency did not copy global_phase when converting a template QuantumCircuit to DAGDependency.
TemplateSubstitution.run_dag_opt constructed the optimized DAGDependency without inheriting the original circuit's global_phase.
When a template carries a nonzero global_phase φ_T (gate content implements e^{-i*φ_T} * I while the full operator is I), the per-match phase contribution was not being subtracted from the output circuit.

Test plan

test_circuit_global_phase_preserved_after_template_match — circuit phase survives a substitution
test_template_nonzero_global_phase_applied_to_circuit — template phase correctly accumulated; includes Operator equivalence check
test_circuit_global_phase_preserved_with_multiple_template_matches — circuit phase preserved across multiple matches

🤖 Generated with Claude Code

…ubstitution circuit_to_dagdependency did not copy global_phase when converting a template QuantumCircuit to DAGDependency, so template phases were lost before matching. TemplateSubstitution.run_dag_opt also constructed a new DAGDependency for the optimised output without inheriting the original circuit's global_phase, resetting it to zero whenever at least one substitution was applied. Fixed by copying global_phase in circuit_to_dagdependency and seeding dag_dep_opt.global_phase from the circuit DAG at construction time. Also added the per-match phase subtraction (Fix 3) for when templates with nonzero global_phase become accepted via the identity check fix in Qiskit#14538. Fixes Qiskit#14537. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…ubstitution circuit_to_dagdependency did not copy global_phase when converting a template QuantumCircuit to DAGDependency, so template phases were lost before matching. TemplateSubstitution.run_dag_opt also constructed a new DAGDependency for the optimised output without inheriting the original circuit's global_phase, resetting it to zero whenever at least one substitution was applied. Additionally, when a template carries a nonzero global_phase phi_T (gate content implements e^{-i*phi_T} * I while the full operator is I), the per-match phase contribution was not being subtracted from the output circuit. Fixes Qiskit#14537. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

qiskit-bot · 2026-04-03T18:20:39Z

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

@Qiskit/terra-core

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

The :issue: role is not available in this project's Sphinx config. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…on test Covers the case where both the circuit and the template carry a nonzero global_phase simultaneously, confirming that fixes Qiskit#2 (circuit phase preserved across substitution) and Qiskit#3 (per-match template phase subtracted) compose correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

ShellyGarion · 2026-04-05T06:57:46Z

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.
This can be idetified by changing the line in this test to target == value:

qiskit/test/python/circuit/test_templates.py

Line 44 in 737060f

self.assertTrue(target.equiv(value))

adcorcol · 2026-04-06T16:14:16Z

Thanks @ShellyGarion!
Extended the fix to rzx_xz, rzx_zz1, rzx_zz2, and rzx_zz3 in PR #15944, which builds on this one

ShellyGarion

LGTM. Thanks for fixing this bug!
I won't merge it yet since I want to see if there are further comments.

alexanderivrii

Thank you for doing this, two minor comments, otherwise LGTM.

…e tests Replace assertTrue(Operator(x) == Operator(y)) with assertEqual(Operator(x), Operator(y)) so that assertion failures show the actual vs expected values rather than just "False is not True". Also shorten multi-line test docstrings to concise one-liners. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

alexanderivrii

LGTM. Julien/Shelly - do you have any further comments?

Cryoris

Thanks for picking this up! I left a few comments, most are typical LLM things like being too verbose, touching unrelated things and being a bit redundant.

Cryoris · 2026-04-08T14:49:30Z

+        # Operator equivalence confirms the full unitary (including phase) is preserved.
+        self.assertEqual(Operator(circuit_in), Operator(result))
+
+    def test_circuit_and_template_both_have_nonzero_global_phase(self):


This tests the same thing as the previous test, right? Do we need it?

I think given that the previous test was changed to 4 gates and therefore ~~avoids the substitution~~ is testing partial match, this test is now testing a different thing, which is the phase subtraction upon full substitution. So I would say we keep it?

- Simplify comment on global phase update in template_substitution.py - Split release note into per-bug files; add fix-circuit-to-dagdependency-global-phase-14537.yaml - Update tests: use issue reproducer (4-gate circuit) in test_template_nonzero_global_phase_applied_to_circuit, merge single/multiple match tests, drop modulo from assertAlmostEqual Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

- Fix test_circuit_and_template_both_have_nonzero_global_phase expected value from np.pi/4 to 7*np.pi/12 (pi/3 + pi/4) - Remove duplicate test_circuit_global_phase_preserved_with_multiple_template_matches - Update test_template_nonzero_global_phase_applied_to_circuit to use Operator equivalence instead of phase/count assertions since partial template match does occur Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Cryoris · 2026-04-09T09:30:52Z

FYI -- I was surprised to see the Claude as co-author didn't have to explicitly sign the CLA. Turns out we have an active rule that excludes bots from requiring to sign (which made sense pre-AI). We'll likely want to discuss in what form such a rule still makes sense before merging this PR.

…4537.yaml Co-authored-by: Julien Gacon <gaconju@gmail.com>

…7.yaml Co-authored-by: Julien Gacon <gaconju@gmail.com>

Co-authored-by: Julien Gacon <gaconju@gmail.com>

Actually I removed all reference to "Regression test" in the test descriptions based on this comment Qiskit#15943 (comment) Co-authored-by: Julien Gacon <gaconju@gmail.com>

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

adcorcol · 2026-04-09T18:08:16Z

FYI -- I was surprised to see the Claude as co-author didn't have to explicitly sign the CLA. Turns out we have an active rule that excludes bots from requiring to sign (which made sense pre-AI). We'll likely want to discuss in what form such a rule still makes sense before merging this PR.

What's your take on #15688, where Claude was also used?

Cryoris · 2026-04-10T09:10:32Z

Commits where LLMs have been used have been merged to Qiskit already, though until now humans (as far as we can tell) were the sole listed author. With signing the CLA they take responsibility that they can give Qiskit the right to use the code.

With the present PR, it's the first time that an LLM is actually listed as co-author. Normally we require all authors to sign the CLA -- yet a bot cannot sign the CLA. So there's an open question on whether our guidelines are explicit enough that you, as human author, take responsibility of the LLMs output by signing the CLA or whether we need something additional. We've discussed this in the team meeting but didn't reach a conclusion yet!

Cryoris

LGTM thanks for the contribution!

adcorcol and others added 2 commits April 3, 2026 13:43

adcorcol requested a review from a team as a code owner April 3, 2026 18:20

style: black reformat test_template_matching.py

bdeeb62

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

adcorcol force-pushed the fix/template-optimization-global-phase-14537 branch from 4e62215 to bdeeb62 Compare April 3, 2026 19:17

docs: use RST hyperlink for issue reference in release note

82ebf1e

The :issue: role is not available in this project's Sphinx config. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

adcorcol mentioned this pull request Apr 3, 2026

fix(library): clifford_6_4 template missing global_phase causes silent rejection #15944

Open

ShellyGarion reviewed Apr 7, 2026

View reviewed changes

alexanderivrii reviewed Apr 7, 2026

View reviewed changes

Comment thread qiskit/converters/circuit_to_dagdependency.py Outdated

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

alexanderivrii reviewed Apr 7, 2026

View reviewed changes

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

alexanderivrii previously approved these changes Apr 8, 2026

View reviewed changes

Cryoris reviewed Apr 8, 2026

View reviewed changes

adcorcol dismissed alexanderivrii’s stale review via 3b0b469 April 8, 2026 17:48

Cryoris reviewed Apr 9, 2026

View reviewed changes

adcorcol and others added 5 commits April 9, 2026 07:03

Update releasenotes/notes/fix-circuit-to-dagdependency-global-phase-1…

51fcdde

…4537.yaml Co-authored-by: Julien Gacon <gaconju@gmail.com>

Update releasenotes/notes/fix-template-optimization-global-phase-1453…

893c47d

…7.yaml Co-authored-by: Julien Gacon <gaconju@gmail.com>

Update test/python/transpiler/test_template_matching.py

12b6284

Co-authored-by: Julien Gacon <gaconju@gmail.com>

Update test/python/transpiler/test_template_matching.py

a925da7

Actually I removed all reference to "Regression test" in the test descriptions based on this comment Qiskit#15943 (comment) Co-authored-by: Julien Gacon <gaconju@gmail.com>

Remove 'Regression test' from docstring per review feedback

bd1833a

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

Cryoris changed the title ~~fix(transpiler): TemplateOptimization drops circuit global_phase on substitution~~ Fix TemplateOptimization dropping the global_phase on substitution Apr 10, 2026

Cryoris added Changelog: Fixed Add a "Fixed" entry in the GitHub Release changelog. mod: transpiler Issues and PRs related to Transpiler labels Apr 10, 2026

Cryoris approved these changes Apr 10, 2026

View reviewed changes

Cryoris added this pull request to the merge queue Apr 10, 2026

Merged via the queue into Qiskit:main with commit f8e543f Apr 10, 2026
28 checks passed

ShellyGarion added this to Qiskit 2.5 Apr 25, 2026

github-project-automation Bot moved this from Ready to Done in Qiskit 2.5 Apr 25, 2026

github-project-automation Bot moved this to Ready in Qiskit 2.5 Apr 25, 2026

ShellyGarion added this to the 2.5.0 milestone Apr 25, 2026

Conversation

adcorcol commented Apr 3, 2026

Summary

Test plan

Uh oh!

qiskit-bot commented Apr 3, 2026

Uh oh!

ShellyGarion commented Apr 5, 2026

Uh oh!

adcorcol commented Apr 6, 2026

Uh oh!

ShellyGarion left a comment

Choose a reason for hiding this comment

Uh oh!

alexanderivrii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexanderivrii left a comment

Choose a reason for hiding this comment

Uh oh!

Cryoris left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Cryoris Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

adcorcol Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Cryoris commented Apr 9, 2026

Uh oh!

adcorcol commented Apr 9, 2026

Uh oh!

Cryoris commented Apr 10, 2026

Uh oh!

Cryoris left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants