-
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
Adapting generators to opmath enabled/disabled #5415
Conversation
…into getattr-ham-lc-aliasing
…into getattr-ham-lc-aliasing
…eAI/pennylane into getattr-ham-lc-aliasing
Hello. You may have forgotten to update the changelog!
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## ham-tests #5415 +/- ##
==========================================
Coverage 99.47% 99.47%
==========================================
Files 400 400
Lines 37146 37143 -3
==========================================
- Hits 36950 36949 -1
+ Misses 196 194 -2 ☔ View full report in Codecov by Sentry. |
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.
I think we can have best of both worlds with the suggestion. Else looks great 👍 Happy to approve with that fixed :)
…into Adapting-generators-to-opmath
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.
I think one pair of legacy
and new
tests can be combined now, but no blocking comments. Looks good to me, glad this is resolved! 🚀
…into Adapting-generators-to-opmath
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.
nice 👌
…into Adapting-generators-to-opmath
…into Adapting-generators-to-opmath
Context: After enabling the new operator arithmetic by default, we want the generators in the source code to return a
LinearCombination
instance or aHamiltonian
instance wherever possible.Description of the Change: The generators touched in this PR are modified so that they return
qml.Hamiltonian
instead ofSum
,Prod
, orSprod
instances. When opmath is enabled,qml.Hamiltonian
points topennylane.ops.op_math.linear_combination.LinearCombination
, and when it is disabled, it points topennylane.ops.qubit.hamiltonian.Hamiltonian
. This ensures that the appropriate instance is used consistently.Note that the generators unchanged in this PR are modified (wherever possible) in #5410 , #5411 , #5412 (including the changelog entry).
Benefits: A more coherent choice depending on whether opmath is enabled or disabled.
Possible Drawbacks: None that I can think of, except that old opmath would be deprecated in the future. Therefore, some precautions that have been taken here (to ensure that tests associated with generators work even with opmath disabled) might become useless.
Related GitHub Issues: None.
[sc-57982]