-
Notifications
You must be signed in to change notification settings - Fork 3k
Ensure template circuits equal the identity including global phase #15944
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
c69ffee
096a4ac
e31b1c8
e583092
3d12ff5
3e86327
b941044
3547dd7
8244352
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,8 @@ | |
|
|
||
| from __future__ import annotations | ||
|
|
||
| from math import pi | ||
|
|
||
| import numpy as np | ||
|
|
||
| from qiskit.circuit import Parameter, QuantumCircuit | ||
|
|
@@ -24,13 +26,14 @@ def rzx_xz(theta: ParameterValueType | None = None): | |
|
|
||
| .. code-block:: text | ||
|
|
||
| global phase: π | ||
| ┌───┐ ┌───┐┌─────────┐┌─────────┐┌─────────┐┌──────────┐» | ||
| 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 ├» | ||
| └───────┘ └──────────┘» | ||
|
Comment on lines
-28
to
32
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! The circuit drawer indeed draws "Rz" instead of "RZ".
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if this drawings should be fixed in all the RZX templates, not only the ones that had global phase issues.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ShellyGarion, would you like to open a "good first issue" about this?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there is already a PR to orgaize the template library #16131 , so let's move this comment there. |
||
| « ┌─────────┐┌─────────┐┌─────────┐ | ||
| «q_0: ┤ RZ(π/2) ├┤ RX(π/2) ├┤ RZ(π/2) ├ | ||
| «q_0: ┤ Rz(π/2) ├┤ Rx(π/2) ├┤ Rz(π/2) ├ | ||
| « └─────────┘└─────────┘└─────────┘ | ||
| «q_1: ───────────────────────────────── | ||
| « | ||
|
|
@@ -50,4 +53,6 @@ def rzx_xz(theta: ParameterValueType | None = None): | |
| qc.rz(np.pi / 2, 0) | ||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we just use
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, changed to |
||
| return qc | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -112,9 +112,7 @@ def run(self, dag): | |
| data = Operator(dagdependency_to_circuit(template)).data | ||
| else: | ||
| data = Operator(template).data | ||
|
|
||
| comparison = np.allclose(data, identity) | ||
|
|
||
|
Comment on lines
-115
to
-117
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we undo these unrelated changes?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, and apologies |
||
| if not comparison: | ||
| raise TranspilerError( | ||
| "A template is a QuantumCircuit() that performs the identity." | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| --- | ||
| fixes: | ||
| - | | ||
| 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>`__. | ||
|
Comment on lines
+4
to
+9
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regarding the sentence: |
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |||||
| """Test the TemplateOptimization pass.""" | ||||||
|
|
||||||
| import unittest | ||||||
| from inspect import getmembers, isfunction | ||||||
|
|
||||||
| import numpy as np | ||||||
| from qiskit.circuit.commutation_library import SessionCommutationChecker as scc | ||||||
|
|
@@ -29,7 +30,9 @@ | |||||
| clifford_3_1, | ||||||
| clifford_4_1, | ||||||
| clifford_4_2, | ||||||
| clifford_6_4, | ||||||
| ) | ||||||
| import qiskit.circuit.library.templates as templib | ||||||
| from qiskit.converters.circuit_to_dag import circuit_to_dag | ||||||
| from qiskit.converters.circuit_to_dagdependency import circuit_to_dagdependency | ||||||
| from qiskit.transpiler import PassManager | ||||||
|
|
@@ -762,6 +765,35 @@ def test_clifford_templates(self): | |||||
| # All of these gates are in the commutation library, i.e. the cache should not be used | ||||||
| self.assertEqual(scc.num_cached_entries(), 0) | ||||||
|
|
||||||
| def test_template_optimization_accepts_all_templates(self): | ||||||
| """Test that TemplateOptimization accepts and applies every template (#14538).""" | ||||||
| all_templates = [o[1]() for o in getmembers(templib) if isfunction(o[1])] | ||||||
| # Build a cost dict covering every gate name that appears in any template, | ||||||
| # supplementing the default dict which is missing some gates (e.g. sx, p, cz). | ||||||
| all_gate_names = { | ||||||
| instr.operation.name for template in all_templates for instr in template.data | ||||||
| } | ||||||
| extra_costs = dict.fromkeys(all_gate_names, 1) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this necessary to test that the template optimization accepts all templates?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, removed this test |
||||||
| for template in all_templates: | ||||||
| # Bind any free parameters to an arbitrary value. | ||||||
| if template.parameters: | ||||||
| template = template.assign_parameters(dict.fromkeys(template.parameters, 0.2)) | ||||||
| with self.subTest(template=template.name): | ||||||
| result = PassManager( | ||||||
| TemplateOptimization([template], user_cost_dict=extra_costs) | ||||||
| ).run(template.copy()) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The explicit copy shouldn't be necessary, since the
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not applicable after removing the test for all templates |
||||||
| # All gates must be cancelled — the template matched itself fully. | ||||||
| # Assumption: the matching algorithm finds a complete self-match for | ||||||
| # every template in the library. This holds for all current templates | ||||||
| # (verified empirically) but if a future template is added for which | ||||||
| # TemplateOptimization cannot find a complete self-match, this assertion | ||||||
| # will fail spuriously and should be replaced with a weaker check such | ||||||
| # as asserting that result.num_operations < template.num_operations. | ||||||
| self.assertEqual(result.count_ops(), {}) | ||||||
| # Full phase equality (not just equiv) confirms that global_phase is | ||||||
| # propagated correctly through the substitution pipeline. | ||||||
| self.assertEqual(Operator(template), Operator(result)) | ||||||
|
|
||||||
| def test_circuit_global_phase_preserved_after_single_and_multiple_template_match(self): | ||||||
| """Test that circuit global_phase survives template optimization (#14537).""" | ||||||
|
|
||||||
|
|
@@ -794,7 +826,6 @@ def test_circuit_global_phase_preserved_after_single_and_multiple_template_match | |||||
|
|
||||||
| def test_template_nonzero_global_phase_applied_to_circuit(self): | ||||||
| """Test the template's global phase is respected (#14537).""" | ||||||
|
|
||||||
| template = QuantumCircuit(1) | ||||||
| template.h(0) | ||||||
| template.s(0) | ||||||
|
|
@@ -813,6 +844,9 @@ def test_template_nonzero_global_phase_applied_to_circuit(self): | |||||
|
|
||||||
| 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) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems that this test is failing CI checks now
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I think here it needs to |
||||||
| self.assertEqual(result.count_ops(), {}) | ||||||
| self.assertEqual(Operator(circuit_in), Operator(result)) | ||||||
|
|
||||||
| def test_circuit_and_template_both_have_nonzero_global_phase(self): | ||||||
|
|
@@ -840,10 +874,49 @@ def test_circuit_and_template_both_have_nonzero_global_phase(self): | |||||
|
|
||||||
| # 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) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This manual modulo shouldn't be necessary (this might be rebasing gone wrong, since we already undid this in the last PR)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed modulo here and in another point in this file |
||||||
| self.assertEqual(result.count_ops(), {}) | ||||||
| self.assertEqual(Operator(circuit_in), Operator(result)) | ||||||
|
|
||||||
| def test_two_global_phase_carrying_template_matches_accumulate_phase(self): | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed as suggested |
||||||
| """Test that phase contributions accumulate correctly when a template matches twice (#14538). | ||||||
|
|
||||||
| Uses clifford_6_4 (SHSHSH, global_phase = -pi/4) as the template. A circuit | ||||||
| containing two SHSHSH blocks separated by a T gate produces two matches; each | ||||||
| match contributes +pi/4 to the circuit's global_phase, leaving pi/2 total. | ||||||
| """ | ||||||
| template = clifford_6_4() | ||||||
|
|
||||||
| qr = QuantumRegister(1, "qr") | ||||||
| circuit_in = QuantumCircuit(qr) | ||||||
| # First SHSHSH block — matches clifford_6_4 | ||||||
| circuit_in.s(qr[0]) | ||||||
| circuit_in.h(qr[0]) | ||||||
| circuit_in.s(qr[0]) | ||||||
| circuit_in.h(qr[0]) | ||||||
| circuit_in.s(qr[0]) | ||||||
| circuit_in.h(qr[0]) | ||||||
| # T gate separates the two matches | ||||||
| circuit_in.t(qr[0]) | ||||||
| # Second SHSHSH block — matches clifford_6_4 again | ||||||
| circuit_in.s(qr[0]) | ||||||
| circuit_in.h(qr[0]) | ||||||
| circuit_in.s(qr[0]) | ||||||
| circuit_in.h(qr[0]) | ||||||
| circuit_in.s(qr[0]) | ||||||
| circuit_in.h(qr[0]) | ||||||
|
|
||||||
| result = PassManager( | ||||||
| TemplateOptimization([template], user_cost_dict={"s": 1, "h": 1, "t": 1}) | ||||||
| ).run(circuit_in) | ||||||
|
|
||||||
| # Both SHSHSH blocks cancelled; only T remains. | ||||||
| self.assertEqual(result.count_ops(), {"t": 1}) | ||||||
| # Each of the two matches contributes +pi/4; total accumulated phase = pi/2. | ||||||
| self.assertAlmostEqual(float(result.global_phase) % (2 * np.pi), np.pi / 2) | ||||||
| # Authoritative check: full unitary (including phase) must be identical. | ||||||
| self.assertEqual(Operator(circuit_in), Operator(result)) | ||||||
|
|
||||||
|
|
||||||
| if __name__ == "__main__": | ||||||
| unittest.main() | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the order of gates is reversed to the order of operators, I think putting
SHSHSHis misleading here since it's unclear what it refers to -- can we just remove that in favor of something simpler likeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, yes, changed to your suggestion.