Update `gate.control()` to return `AnnotatedOperation` #11433

alexanderivrii · 2023-12-19T14:33:43Z

Summary

Partially resolves #11082.

This PR adds a new argument annotated to all the "control" methods, that is to QuantumCircuit.control(), Gate.control() and all of the subclasses of Gate, for example to SwapGate.control() and UnitaryGate.control().

When annotated is False (which is the default), the behavior is unchanged, making the change backward-compatible. As an example, SwapGate().control(1, annotated=False) returns an object of type CSwapGate (or, more precisely, a singleton version of that), while SwapGate().control(2, annotated=False) returns an object of type ControlledGate.

When annotated is True, the control method is allowed to return an AnnotatedOperation instead, leading to a more abstract representation that avoids eagerly constructing the gate's definition and allows potential "high-level" transpiler optimizations. I am saying "is allowed" because in practice there may be cases where we wouldn't want to introduce this abstraction, for example we may decide that a singly-controlled XGate should always be a CXGate and not an annotated operation. This will make even more sense when we try to similarly extend the power and inverse methods: it makes no sense to say that the inverse of a CXGate is an annotated operation and not just a CXGate.

The plans are to follow this PR in two directions.

First, we want to introduce similar capability for inverse and power methods, however each of these comes with its own set of headaches, so I have limited this PR only to control.

Second, we want to identify where in Qiskit it's beneficial to call control() with anotated=True. I believe it would be useful to do so in various circuit-constructing utilities (for example constructing controlled QFT-based adders), but there could be more opportunities.

Feedback regarding the approach is welcome. I am explicitly tagging @jakelishman and @mtreinish (especially that this is based on Matthew's offline suggestion).

qiskit-bot · 2023-12-19T14:33:48Z

One or more of the the following people are requested to review this:

@Cryoris
@Qiskit/terra-core
@ajavadia

coveralls · 2023-12-20T12:08:17Z

Pull Request Test Coverage Report for Build 7730318921

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

-14 of 113 (87.61%) changed or added relevant lines in 19 files are covered.
22 unchanged lines in 5 files lost coverage.
Overall coverage increased (+0.007%) to 89.588%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/circuit/library/generalized_gates/unitary.py	7	8	87.5%
qiskit/circuit/library/standard_gates/u1.py	13	14	92.86%
qiskit/transpiler/passes/synthesis/high_level_synthesis.py	3	5	60.0%
qiskit/circuit/annotated_operation.py	2	5	40.0%
qiskit/circuit/library/standard_gates/x.py	26	29	89.66%
qiskit/circuit/library/generalized_gates/mcmt.py	1	5	20.0%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	2	91.94%
qiskit/primitives/containers/estimator_pub.py	2	97.47%
qiskit/primitives/containers/sampler_pub.py	2	96.92%
crates/qasm2/src/parse.rs	6	97.62%
qiskit/primitives/containers/bindings_array.py	10	92.31%

Totals
Change from base Build 7728099338:	0.007%
Covered Lines:	59358
Relevant Lines:	66257

💛 - Coveralls

mtreinish

Overall I think this looks good. I've only done a high level review, quick skim over the code so far. But before I dive in line by line I had a couple of high level questions:

Do you think there is a time in the future where we want to have annotated=True by default?
For concrete gates that have a defined controlled variant (e.g. X -> CX) is there an advantage to calling XGate().control(annotated=True)? I assume this opens up opportunities for optimization by having things defined abstractly with AnnotatedOperation.

I'm just wondering if there are potentially different defaults we want here as a breaking change for the 1.0.0 release? Like I'm wondering if doing the base Gate.control default to annotated=True makes more sense. From a backwards compatibility PoV what you have here is better, but I'm just curious of your thoughts on the longer term plan with this.

qiskit/circuit/library/generalized_gates/unitary.py

alexanderivrii · 2024-01-16T10:55:03Z

These are really great questions.

Longer-term I would indeed like to see Gate.control, QuantumCircuit.control and UnitaryGate.control return AnnotatedOperations by default.

I have now run local tests to see what would break if we were to set annotated=True in the three methods above, and there are 76 failing tests (a bit more than I have hoped). On a quick inspection:

Some of these are related to QPY and should hopefully be fixed by Fix qpy support for Annotated Operations #11505.
Some of these are related to visualization. Most of the visualization problems were solved in Add drawer support for AnnotatedOperations #11202 and Fix text drawer AnnotatedOperations #11466, but apparently there are still some corner-cases remaining.
Many of these are related to testing controlled gates, and fail because the returned object has no definition, label, inverse or is not an instance of ControlledGate. Though the inverse would be fixed by the follow-up PR on inverse gates. A simple fix would be to change the tests for controlled gates that explicitly mention label, definition or inverse to call control with annotated=False.
And there might be a few more problems (out of these 76 ones). I will take a deeper look.

I am up to making this breaking change for Qiskit 1.0 provided that we can quickly resolve all the problems above.

Regarding controlled gates with defined controlled variants. At the moment I can't think of a convincing use-case when having an X-gate with a control modifier representing a single control might be better than just having a CX-gate (or an MCX-gate in case of multiple controls), however I do agree that this possibly opens a door to some optimizations.

Update: after thinking about the above, I think that always returning a CX/MCX-gate for X.control() might indeed be a better option, as this does not prevent the possibility of a transpiler pass that translates MCX-gates to AnnotatedGates (if the goal is to maybe combine gates with similar control modifiers).

alexanderivrii · 2024-01-16T15:20:37Z

I have taken a close look at all tests that would fail if we changed the default to control(annotated=True).

As I wrote before, most of the failing tests pertain to testing controlled gates, as annotated operations do not have methods label or definition. These tests would be easy to adapt.

There are failing tests related to the Unroller pass as it does not support circuits with AnnotatedOperations (as these have no definition), however the pass is being deprecated, so these tests can probably be simply removed.

There are some failing visualization tests: thanks to #11202 the annotated operations with control modifiers are printed very nicely and quite similarly to controlled gates, but still there are some minor differences in circuit_test_drawer: the following code

ghz_circuit = QuantumCircuit(3, name="ghz")
ghz_circuit.h(0)
ghz_circuit.cx(0, 1)
ghz_circuit.cx(1, 2)
ghz = ghz_circuit.to_gate()
cghz = ghz.control(1, ctrl_state="0")
circuit = QuantumCircuit(4)
circuit.append(cghz, [3, 1, 0, 2])

used to print

     ┌──────┐
q_0: ┤1     ├
     │      │
q_1: ┤0 ghz ├
     │      │
q_2: ┤2     ├
     └──┬───┘
q_3: ───o────

and now it's exactly the same picture except that the first letter in ghz is capitalized. There are also some difference with (not) printing parameters of the base operation.

The two remaining failing tests are test.python.circuit.test_controlled_gate.TestControlledGate.test_assign_nested_controlled_cu and test.python.circuit.test_circuit_operations.TestCircuitOperations.test_control and they point to a real problem for annotated gates over parameterized gates (and I now recall that @jakelishman mentioned at some point that this may be an issue). For example the following code

theta = Parameter("t")
qc_c = QuantumCircuit(2)
qc_c.crx(theta, 1, 0)
custom_gate = qc_c.to_gate().control()
qc = QuantumCircuit(3)
qc.append(custom_gate, [0, 1, 2])
assigned = qc.assign_parameters({theta: 0.5})

does not currently assign parameters to the base operation of an annotated operation.

mtreinish

Overall this LGTM, I think you're right that even with a potentially breaking change in qiskit 1.0 the limitations around AnnotatedOperation (primarily the QPY support still needed) is enough of a reason to be conservative here and default to the existing behavior. We can revisit this for qiskit 2.0.0 if we want to potentially move everything to be a annotated operation.

I left a few inline comments but nothing major and I'm fine merging this as is.

mtreinish · 2024-01-30T18:16:39Z

qiskit/circuit/annotated_operation.py

+        num_ctrl_qubits: int = 1,
+        label: str | None = None,
+        ctrl_state: int | str | None = None,
+        annotated: bool = False,


Should this be?

Suggested change

annotated: bool = False,

annotated: bool = True,

Not that it matters much, but we're always returning an annotated op here.

Good point. I have also simplified the returned gate: instead of returning a control-annotated operation on top of the current annotated operation, we return an annotated operation with the current set of modifiers extended with the new control modifiers.

mtreinish · 2024-01-30T18:17:10Z

qiskit/circuit/annotated_operation.py

+        label: str | None = None,
+        ctrl_state: int | str | None = None,
+        annotated: bool = False,
+    ):


?

Suggested change

):

) -> AnnotatedOperation:

mtreinish · 2024-01-30T18:24:27Z

qiskit/circuit/library/generalized_gates/mcmt.py

+        if not annotated:
+            if ctrl_state is None:  # TODO add ctrl state implementation by adding X gates
+                gate = MCMT(
+                    self.gate, self.num_ctrl_qubits + num_ctrl_qubits, self.num_target_qubits
+                )
+            else:
+                gate = super().control(num_ctrl_qubits, label, ctrl_state, annotated=annotated)
+        else:
+            gate = AnnotatedOperation(
+                self, ControlModifier(num_ctrl_qubits=num_ctrl_qubits, ctrl_state=ctrl_state)
+            )


I'd probably have done to save some code and reduce duplication:

Suggested change

if not annotated:

if ctrl_state is None: # TODO add ctrl state implementation by adding X gates

gate = MCMT(

self.gate, self.num_ctrl_qubits + num_ctrl_qubits, self.num_target_qubits

)

else:

gate = super().control(num_ctrl_qubits, label, ctrl_state, annotated=annotated)

else:

gate = AnnotatedOperation(

self, ControlModifier(num_ctrl_qubits=num_ctrl_qubits, ctrl_state=ctrl_state)

)

if not annotated and ctrl_state is None:

gate = MCMT(

self.gate, self.num_ctrl_qubits + num_ctrl_qubits, self.num_target_qubits

)

else:

gate = super().control(num_ctrl_qubits, label, ctrl_state, annotated=annotated)

I think this applies pretty much everwhere.

Oh, I absolutely love this suggestion. Not only this makes the code shorter and more readable, it also seems a more correct way to apply the control method. Changed across the board in 0783fbb.

mtreinish

LGTM, thanks for the updates

alexanderivrii added 3 commits December 18, 2023 14:33

changing control method for gate and annotated op

ef8ee8a

adding argument annotated to all control functions

d8e64ed

formatting

bc977ab

alexanderivrii requested review from ShellyGarion and a team as code owners December 19, 2023 14:33

lint

0ba4e15

alexanderivrii added 2 commits December 20, 2023 20:56

controlled library gates when annotated=True and tests

861a327

release notes

6a17957

alexanderivrii changed the title ~~[WIP] Update gate.control() to return AnnotatedOperation~~ Update gate.control() to return AnnotatedOperation Dec 25, 2023

alexanderivrii mentioned this pull request Dec 25, 2023

Update Gate.control method to return AnnotatedOperation #11082

Closed

making sure all control functions have similar signature

d827990

1ucian0 assigned mtreinish Jan 8, 2024

1ucian0 added this to the 1.0.0 milestone Jan 11, 2024

1ucian0 added the priority: medium label Jan 11, 2024

mtreinish reviewed Jan 11, 2024

View reviewed changes

qiskit/circuit/library/generalized_gates/unitary.py Show resolved Hide resolved

alexanderivrii mentioned this pull request Jan 18, 2024

Update gate.inverse() to return AnnotatedOperation #11593

Merged

mtreinish previously approved these changes Jan 30, 2024

View reviewed changes

alexanderivrii added 2 commits January 31, 2024 15:34

suggestions from code review + improved support for annotated.control

268b265

simplifying logic based on code review

0783fbb

alexanderivrii dismissed mtreinish’s stale review via 0783fbb January 31, 2024 14:30

alexanderivrii added 2 commits January 31, 2024 17:14

Merge branch 'main' into controlled-to-annotated

d836fab

merge conflicts fix

33f9983

mtreinish approved these changes Jan 31, 2024

View reviewed changes

mtreinish enabled auto-merge January 31, 2024 18:09

mtreinish added this pull request to the merge queue Jan 31, 2024

Merged via the queue into Qiskit:main with commit 0364786 Jan 31, 2024
12 checks passed

mtreinish mentioned this pull request Feb 1, 2024

Expand docstrings for annotated flag on control() and inverse() #11707

Closed

alexanderivrii mentioned this pull request Mar 29, 2024

Adding annotated argument to power methods #12101

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `gate.control()` to return `AnnotatedOperation` #11433

Update `gate.control()` to return `AnnotatedOperation` #11433

alexanderivrii commented Dec 19, 2023 •

edited

Loading

qiskit-bot commented Dec 19, 2023

coveralls commented Dec 20, 2023 •

edited

Loading

mtreinish left a comment

alexanderivrii commented Jan 16, 2024

alexanderivrii commented Jan 16, 2024

mtreinish left a comment

mtreinish Jan 30, 2024

alexanderivrii Jan 31, 2024

mtreinish Jan 30, 2024

alexanderivrii Jan 31, 2024

mtreinish Jan 30, 2024

alexanderivrii Jan 31, 2024

mtreinish left a comment

Update gate.control() to return AnnotatedOperation #11433

Update gate.control() to return AnnotatedOperation #11433

Conversation

alexanderivrii commented Dec 19, 2023 • edited Loading

Summary

qiskit-bot commented Dec 19, 2023

coveralls commented Dec 20, 2023 • edited Loading

Pull Request Test Coverage Report for Build 7730318921

Warning: This coverage report may be inaccurate.

💛 - Coveralls

mtreinish left a comment

Choose a reason for hiding this comment

alexanderivrii commented Jan 16, 2024

alexanderivrii commented Jan 16, 2024

mtreinish left a comment

Choose a reason for hiding this comment

mtreinish Jan 30, 2024

Choose a reason for hiding this comment

alexanderivrii Jan 31, 2024

Choose a reason for hiding this comment

mtreinish Jan 30, 2024

Choose a reason for hiding this comment

alexanderivrii Jan 31, 2024

Choose a reason for hiding this comment

mtreinish Jan 30, 2024

Choose a reason for hiding this comment

alexanderivrii Jan 31, 2024

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

Update `gate.control()` to return `AnnotatedOperation` #11433

Update `gate.control()` to return `AnnotatedOperation` #11433

alexanderivrii commented Dec 19, 2023 •

edited

Loading

coveralls commented Dec 20, 2023 •

edited

Loading