-
Notifications
You must be signed in to change notification settings - Fork 37
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
depr: remove legacy opmath #283
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #283 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 7 7
Lines 1251 1244 -7
Branches 163 161 -2
=========================================
- Hits 1251 1244 -7 ☔ View full report in Codecov by Sentry. |
@@ -1314,6 +1313,8 @@ def test_execute_some_samples(mock_run): | |||
mock_run.return_value = task | |||
dev = _aws_device(wires=3) | |||
|
|||
print(type(qml.Hadamard(0) @ qml.Identity(1))) |
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.
nit: Do we need this print, should this type check be asserted instead?
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.
Ah, we don't need this at all!
A previous test (now removed) switched the default behaviour of the @
operator to the legacy behaviour and forgot to switch it back, so this test was actually running with the legacy code instead of the current code and creating an incorrect operator type. I just added this when I was debugging what was going on there.
But it's fixed now, and the legacy code doesn't exist anymore, so I'll just delete it completely.
@_translate_observable.register | ||
def _(t: qml.operation.Tensor): | ||
return reduce(lambda x, y: x @ y, [_translate_observable(factor) for factor in t.obs]) | ||
|
||
|
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.
For my understanding, why do we remove this, is this now internally handled?
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.
Tensor
is an operator class that has been marked as legacy for a long time, and has already been deprecated. Now we are completing its removal. So qml.operation.Tensor
just won't exist anymore, and doesn't need any handling.
For failing title check, you need to use |
Description of changes:
PennyLane has removed support for legacy operator arithmetic (Hamiltonian, Tensor). These will part of the upcoming release.
Testing done:
Removed tests referring to legacy operator arithmetic. No additional tests needed - we are removing support.
Merge Checklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.General
Tests
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.