-
Notifications
You must be signed in to change notification settings - Fork 586
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
Add symbolic base class for operator arithmetic #2721
Conversation
Hello. You may have forgotten to update the changelog!
|
Codecov Report
@@ Coverage Diff @@
## master #2721 +/- ##
==========================================
- Coverage 99.61% 99.61% -0.01%
==========================================
Files 255 256 +1
Lines 20913 20864 -49
==========================================
- Hits 20833 20784 -49
Misses 80 80
Continue to review full report at Codecov.
|
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.
Thanks @albi3ro for this change 💯 It will be really useful for the operator arithmetics. I have a couple of questions, I will be happy to approve it after that 👍
Also I think the codecov is failing because you reduced the number of line to be tested in the code base. Therefore we should probably skip the test when it is ready to be merged. |
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.
It looks good to me 💯 But we will probably need @antalszava to bypass the failing coverage check
:meth:`~.operation.Operator.eigvals`, and :meth:`~.operation.Operator.decomposition`. | ||
""" | ||
|
||
_name = "Symbolic" |
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.
Why define the name like this? Is it to ensure that every instance of SymbolicOp
will have the same cls._name
attribute?
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.
This needed to be defined so that we could instantiate the class and inspect it without an error occurring. Child classes should override this in their __init__
function. I didn't want to define this during SymbolicOp.__init__
, since I didn't want to accidentally override the _name
property of the child class.
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.
Looks good to me! Great job removing a bunch of redundant code! I can definitely use this in the ScalarProd class implementation! Two thoughts:
- How do you feel about the name of this class? To me it's not obvious that this class is generalizing single operator modifying operations. The term SymbolicOp is pretty general and can apply to addition, multiplication, etc. Could we try and brain storm a more explicit name? Maybe
OperatorModifier
?SingleOpModifier
? Maybe we can ask the PennyLane channel about this - From a testing perspective, since these methods are being tested here, if we were to subclass from this op, and then use the
super().__init__(...)
method, would we still need to test some of the properties and attributes that are being generalized here?
This PR adds a base class
SymbolicOp
which contains a single operator and defers all operator properties to those of the base class. In the future, this class may either be altered or extended to account for symbolic ops with more than one base, such as the upcomingSum
andProd
.For the time being, this class will reduce duplicated code for:
Adjoint
(already in master)Pow
(already in master)Controlled
(review ready PR)ScalarProd
(WIP PR)Exp
(upcoming)