-
Notifications
You must be signed in to change notification settings - Fork 604
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
Legacy device API handles Prod observables #5475
Conversation
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 @@
## master #5475 +/- ##
==========================================
- Coverage 99.67% 99.66% -0.01%
==========================================
Files 404 404
Lines 37833 37567 -266
==========================================
- Hits 37709 37442 -267
- Misses 124 125 +1 ☔ 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.
Great work! Thanks @astralcai !
I left mostly questions for now :)
Concerning the renaming of the mock devices in the test I dont have an opinion and would refer to someone from core to judge whether changing all of them is a good idea or not.
@Qottmann The renaming was because multiple test cases were using different mock devices that were exact duplicates of one another. I removed the duplicates, and changed all these test cases to use just the one that was left. |
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 @astralcai! Left a couple of small comments about tests.
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 @astralcai
My questions are mostly answered and I am overall happy with the PR.
The only thing left to discuss is the simplify. Why do we need it?
@Qottmann The purpose of this is to expand a nested |
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 for now, happy to move on and revisit later for a better solution with simplify 👍
Thanks @astralcai great work!
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.
What @Qottmann said 🚀
Context:
The legacy device API has special handling of
Tensor
andSum
as observables, butProd
is not covered by that.Description of the Change:
Prod
observables are expanded if they simplify to aSum
check_validity
, the operands ofProd
observables are checked.Possible Drawbacks:
simplify
is called onProd
observables many times, which might be inefficientRelated Shortcut Story:
[sc-60584]