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

quantum_info.operator mixin interface refactor #5617

Merged
merged 20 commits into from
Feb 8, 2021

Conversation

chriseclectic
Copy link
Member

@chriseclectic chriseclectic commented Jan 12, 2021

Summary

Internal API changes to BaseOperator class to simplify making operator subclasses, and reduce copy paste of API docs.

Details and comments

Refactors the quantum_info BaseOperator and subclasses to use mixins for defining the operator inferface. This allows operator subclasses to only include the mixin for interfaces they support. For example Operator and QuantumChannel operators support linear operations (addition, subtraction), but Clifford and Pauli operators do not.

Current mixins are:

  • GroupMixin: this defines the basic API to be compatible with QuantumCircuits. It includes compose, dot, tensor, expand
  • AdjointMixin: defines conjugate, transpose, adjoint methods.
  • MultiplyMixin: this is not part of the BaseOperator and can be used by special operators which support scalar multiplication. It defines __mul__ and __truediv__ for scalars.
  • LinearMixin: this defines linear operators which support addition, subtraction, and scalar multiplication. It includes the MultiplyMixin, plus defines __add__ and __sub__ methods.
  • ToleranceMixin: this defines the atol and rtol properties for the operator for use in numeric comparisons

BaseOperator only uses GroupMixin for its API. Other mixins can be included by subclasses which support their interface or require their functionality. A LinearOp abstract class was also added that includes LinearMixin, AdjointMixin and TolerancesMixin with BaseOperator.

This PR also refactors a lot of doc strings in the operator classes to avoid unnecessary duplication in derived classes. This is handled by the generate_apidocs function that must be called on a class after its definition (Due to however python handles abstractmethod doc strings I couldn't seem to get this function to work as a decorator when defining the class).

@chriseclectic chriseclectic added the on hold Can not fix yet label Jan 12, 2021
@chriseclectic chriseclectic added this to the 0.17 milestone Jan 12, 2021
@chriseclectic chriseclectic requested a review from a team as a code owner January 12, 2021 22:31
@chriseclectic chriseclectic added Changelog: API Change Include in the "Changed" section of the changelog and removed on hold Can not fix yet labels Jan 20, 2021
@chriseclectic chriseclectic changed the title [WIP] quantum_info.operator mixin interface refactor quantum_info.operator mixin interface refactor Jan 20, 2021
@ikkoham ikkoham mentioned this pull request Feb 2, 2021
@molar-volume
Copy link
Contributor

Maybe

def _evolve(self, state, qargs=None):
        return SuperOp(self)._evolve(state, qargs)

could be moved to QuantumChannel, since it is same for all subclasses (except SuperOp).

@chriseclectic
Copy link
Member Author

chriseclectic commented Feb 2, 2021

Maybe

def _evolve(self, state, qargs=None):
        return SuperOp(self)._evolve(state, qargs)

could be moved to QuantumChannel, since it is same for all subclasses (except SuperOp).

That is True, however it has cyclic import issues which is why it isn't in the base class.

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.

Looks good overall, few small comments.

qiskit/quantum_info/operators/mixins/gate_op.py Outdated Show resolved Hide resolved
qiskit/quantum_info/operators/mixins/__init__.py Outdated Show resolved Hide resolved
qiskit/quantum_info/operators/mixins/group.py Show resolved Hide resolved
@ikkoham
Copy link
Contributor

ikkoham commented Feb 4, 2021

Why do private and public methods coexist? (e.g. _compose, compose)
_compose is an abstract method, so it has to be defined by the user. But at the same time, if user also overrides compose, then __matmul__ will become the compose (not _compose). Is this the desired behavior?

@chriseclectic
Copy link
Member Author

Why do private and public methods coexist? (e.g. _compose, compose)
_compose is an abstract method, so it has to be defined by the user. But at the same time, if user also overrides compose, then __matmul__ will become the compose (not _compose). Is this the desired behavior?

The idea was user never overrides compose only _compose. Honestly this was mostly to prevent having to have the bit of code:

        if qargs is None:
            qargs = getattr(other, 'qargs', None)

in every subclasses compose method. But maybe thats not a good enough reason.

@chriseclectic
Copy link
Member Author

I updated to remove the _tensor and _compose methods and go back to the original way of have compose, expand, tensor all abstract methods. Thanks to review feedback I think the added complexity wasn't worth the small code reduction.

@kdk kdk merged commit d29d691 into Qiskit:master Feb 8, 2021
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Feb 15, 2021
In Qiskit#5617 a single usage of np.int slipped in to the clifford module.
This type is deprecated in numpy 1.20.0 and is just an alias for
python's stdlib int type (see Qiskit#5788 and Qiskit#5758 for the earlier fixes).
This usage is causing downstream failures in other project's CI that
use this module in environment's where warnings aren't allowed (mainly
ignis's docs builds). This commit fixes this stray np.int usage to
unblock CI and remove the deprecation warning from anything using the
clifford module.
mergify bot pushed a commit that referenced this pull request Feb 15, 2021
In #5617 a single usage of np.int slipped in to the clifford module.
This type is deprecated in numpy 1.20.0 and is just an alias for
python's stdlib int type (see #5788 and #5758 for the earlier fixes).
This usage is causing downstream failures in other project's CI that
use this module in environment's where warnings aren't allowed (mainly
ignis's docs builds). This commit fixes this stray np.int usage to
unblock CI and remove the deprecation warning from anything using the
clifford module.

Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the "Changed" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants