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

Operation abstract base class #7087

Merged
merged 95 commits into from
Jan 11, 2022
Merged

Conversation

ShellyGarion
Copy link
Member

@ShellyGarion ShellyGarion commented Sep 30, 2021

Summary

Add Operation mixin class and refactor the other classes accordingly.

#5811

co-authored by @alexanderivrii

Details and comments

  • add Operation class
  • update Gate class
  • update Barrier, Reset, Measure classes
  • update QuantumCircuit class - this will be done in a separate PR (deprecating circuit.to_instruction)

In quantum_info:

  • update Clifford class
  • update CNOTDihedral class
  • update pauli class

In extension: need to update classes which are Instruction and not Gate

  • Initialize
  • Isometry

In circuit.library.standard_gates: all are Gate so no update is needed
In circuit.library.generalized_gates: all are QuantumCircuit so no update is needed

In future PR - deprecate Instruction!

@ShellyGarion ShellyGarion requested a review from a team as a code owner September 30, 2021 13:10
qiskit/circuit/instructionset.py Outdated Show resolved Hide resolved
qiskit/circuit/circuit_element.py Outdated Show resolved Hide resolved
@ShellyGarion ShellyGarion requested review from 1ucian0 and kdk October 7, 2021 12:39
Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great so far! One comment on the .setters for Instruction, which relates to whether or not we can or should use CircuitElement for QuantumCircuit: once these elements are added to a circuit (and in some cases even before), these properties should effectively be fixed.

While e.g. it is currently possible to change Instruction.name or Instruction.num_qubits (because it's python and we haven't taken the steps extra to make them read-only), this will only lead to confusion or an invalid circuit in 99% of the cases (as in @nkanazawa1989 's issue from the other day). I'd have to think a bit about how this could be implemented through the CircuitElement mixin.

qiskit/circuit/instruction.py Show resolved Hide resolved
qiskit/circuit/instruction.py Show resolved Hide resolved
qiskit/circuit/instruction.py Outdated Show resolved Hide resolved
qiskit/circuit/instruction.py Outdated Show resolved Hide resolved
@eliarbel
Copy link
Contributor

I agree with Chris that we should use a better way than overriding the __new__ method for preventing instantiation of Operation. I think however that maybe we can change only the name method to become @abstractmethod to achieve this and leave the other methods (if we indeed need them to be in Operations) not abstract, to keep all the derived classes code cleaner, i.e. avoid re-implementing all those getters over and over again. In general mixins can (and actually should) bring their own functionality (including attributes if needed) to be "attached" to their derived classes, as otherwise they are not really mixins but rather just interfaces. For the long run, when we deprecate Instruction some of its functionality can move over to Operation so we may see more logic and data in Operation.
Another question is whether we really need all the current attributes in Operation. name and num_qubits (I think clbits as well) seem quite basic requirement for any circuit element, but maybe operands should be present only in the relevant gates, below Operation in the hierarchy?

@jakelishman
Copy link
Member

From the meeting: we decided that "something that can be added to a circuit" is a description of an interface, not an implementation, and consequently we should just look to define the interface. This means:

  • there should be no __new__ or __init__ methods on this interface
  • name, num_qubits and num_clbits should become abstractmethods (as well as property) with no definition (raise NotImplemented is fine)
  • num_operands and operands can be removed for now, since they're not 100% necessary
  • the class should set __slots__ = () right at the top, underneath the docstring

In general, the reasons for these all stem from how we plan to use this as an interface attached to other objects. This means that it needs to be well-behaved in a multiple-inheritance context. The primary implication is that the class should be as lightweight as absolutely possible, which means removing all concrete behaviour and leaving only the required interface for children to fill in.


More detail

Requiring the underlying data to be stored in a certain name (here set by __init__ and accessed by e.g. num_qubits) increases the chance of clashes with other classes, but also has memory-usage and speed implications in Python. This is because the class either requires the creation of __dict__ (the current behaviour), which is slower and more memory-hungry when many objects are instantiated, or it needs to define a non-empty __slots__ to store the attributes, which prevents the class from being mixed in with a more concrete parent that defines a non-empty __slots__.

The original reason for having __init__ was to ease duplication in child classes. In practice, this will not be a problem because we will still have (for example) the Gate class, which is a concrete implementation of this interface that other concrete implementations will inherit from. Having it simply be an interface means that we will be able to add things that aren't Gate (or whatever), and those classes will no longer need to tie themselves in knots trying to call a somewhat Gate-specific __init__ method.

Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @ShellyGarion . Few minor comments, otherwise looks good!

qiskit/circuit/gate.py Outdated Show resolved Hide resolved
qiskit/circuit/library/standard_gates/x.py Outdated Show resolved Hide resolved
These objects include :class:`~qiskit.circuit.Gate`, :class:`~qiskit.circuit.Reset`,
:class:`~qiskit.circuit.Barrier`, :class:`~qiskit.circuit.Measure`,
and operators such as :class:`~qiskit.quantum_info.Clifford`.
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and in the release note, it would be good to have a simple example of how users should expect to use this class and some details on its motivation. e.g. Clifford is probably a more representative example than Reset.

test/python/circuit/gate_utils.py Outdated Show resolved Hide resolved
@ShellyGarion ShellyGarion requested review from kdk and removed request for ewinston January 11, 2022 15:06
Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates here @ShellyGarion and @alexanderivrii ! I think there are still a few open questions around how we ultimately want to build around Operation (like how we want to define and handle params/operands, and how to set a deprecation path for Instruction) but I think these can be discussed separately. This is sufficient to unblock e.g. #7238 and the related https://github.com/Qiskit/qiskit-terra/labels/synthesis work so this LGTM.

@mergify mergify bot merged commit 05b60a5 into Qiskit:main Jan 11, 2022
@mtreinish mtreinish added Changelog: None Do not include in changelog and removed Changelog: New Feature Include in the "Added" section of the changelog labels Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.