Skip to content
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

Update Gate.control method to return AnnotatedOperation #11082

Closed
alexanderivrii opened this issue Oct 23, 2023 · 4 comments · Fixed by #11433
Closed

Update Gate.control method to return AnnotatedOperation #11082

alexanderivrii opened this issue Oct 23, 2023 · 4 comments · Fixed by #11433
Assignees
Labels
type: feature request New feature or request
Milestone

Comments

@alexanderivrii
Copy link
Contributor

alexanderivrii commented Oct 23, 2023

What should we add?

Continuing the saga of supporting annotated operations in Qiskit, we should update the Gate.control() method to return AnnotatedOperation instead of a ControlledGate and in particular to avoid eagerly constructing the controlled gate's definition.

Note that for classes that are derived from Gate and that implement the control method, this said implemented method should be used.

Examples:

  • SwapGate().control(1) should still produce CSwapGate,
  • SwapGate.control(2) (which ended up calling the control method of its parent class Gate) should produce AnnotatedGate instead of ControlledGate.

There are several immediate problems that I don't know how to handle correctly:

  • This is not backward-compatible as it changes the behavior of the method. We might have code that applies .control() to a gate and expects to see a ControlledGate in return. Additionally, gate.control() accepts label as an argument while AnnotatedOperations do not have labels.
  • We need to update every consumer of QuantumCircuit to handle the case that not everything is an Instruction (in particular, not every Operation may have fields such as base_class, label, params, etc.)

Suggestions?

E.g., maybe we should not try to replace .control but define some new method?

@alexanderivrii alexanderivrii added the type: feature request New feature or request label Oct 23, 2023
@alexanderivrii alexanderivrii self-assigned this Oct 23, 2023
@alexanderivrii alexanderivrii modified the milestones: 0.45.0, 0.46.0 Oct 23, 2023
@ewinston
Copy link
Contributor

ewinston commented Nov 6, 2023

If the goal is to ultimately handle controlled gate translation in the transpiler can't control() always return AnnotatedGate? For instance I'm not sure I understand the need for keeping around CSwapGate rather than moving its implementation to transpile time like other AnnotatedGate objects where one has knows the ultimate target basis.

@jakelishman
Copy link
Member

Some of the intention is that there's a difference between a higher-level "abstract" control on an inner gate (useful for optimisations) and a "concrete" gate that a backend might use natively. cx is a classic example of the latter; a backend wants to be able to be able to say "I can implement cx" and it should be representable as an element of a low-level ISA, whereas "annotated gate" is a high/intermediate-level quantum representation for the compiler.

It's questionable to me whether cswap is likely to ever be part of some backend's ISA, but I have a vague recollection that there are architectures that might support it natively. Things like mcx are far more likely to be removed in favour of the more generic annotated operations, since mcx at the moment primarily serves as a way to define a synthesis, which should be the domain of the compiler, not the circuit construction.

@alexanderivrii
Copy link
Contributor Author

Thanks @ewinston, @jakelishman, these are all very good comments.

I feel that always returning AnnotatedOperation is not backward-compatible, and there may be some external code that would break (e.g., if the code expects SwapGate().control(1) to have the definition field). Instead (based on a suggestion by @mtreinish) in #11433 I am adding a new argument annotated to all of the control methods. The idea is that it's false by default making the code backward-compatible, while we will investigate when it's safe to set it to True. Feedback on this is highly welcome.

I think that having specialized classes for special gates is a good idea even when the gates don't end up being natively supported by the backend. For instance, if we had a special synthesis technique for CSwapGates, then keeping a gate as a CSwapGate would allow to apply that synthesis technique through high-level synthesis plugin mechanism.

@alexanderivrii
Copy link
Contributor Author

One more comment, I feel that having gate-specific "control" methods like we have right now is also a good idea, with SwapGate knowing that its 1-controlled version is a CSwapGate, or MCX gate knowing that it's arbitrarily-controlled-version is also a MCX gate. However, I am not 100% sure when exactly this information is already used in Qiskit. As far as I understand, adding "control" to a quantum circuit only uses add_control, which in turn unrolls the gate into "p", "u", "x", "z", "rx", "ry", "rz", "cx" basis gates, and creates MCPhaseGates, etc.. I.e. gate-specific "control' methods are not used during transpilation and only when the circuit is originally constructed -- is this correct?

@jakelishman jakelishman modified the milestones: 0.46.0, 1.0.0 Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature request New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants