-
Notifications
You must be signed in to change notification settings - Fork 603
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
Automatically test plxpr capture integration with assert_valid
#5686
Conversation
Co-authored-by: David Wierichs <david.wierichs@xanadu.ai>
…neAI/pennylane into plxpr-capture-operations
…nylane into plxpr-capture-operations
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.
LGTM, just a few tiny questions. Thanks for the patch @albi3ro!
I'm wondering: Can we have a systematic way to make sure that assert_valid
is called on all PL objects it can be applied to? 🤔
Similar to the logic in the templates testing file, where PL is scraped for Operator
s (and SymbolicOp
s?) and assert_valid
keeps track of which classes it saw? 🤔
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 for the explainers! 🙏
So by default we do automatically validate all operators except for templates. See this file for the automatic collection: https://github.com/PennyLaneAI/pennylane/blob/master/tests/ops/functions/conftest.py We opted templates out of this process due to the complexity of automatically generating a valid instance, but most templates have an |
Ah, brilliant! So the only thing one could add is a cache that memorizes tested templates (from the template test files) and checks for untested templates (as per |
Co-authored-by: David Wierichs <david.wierichs@xanadu.ai>
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.
Two new comments I had.
…nnyLaneAI/pennylane into plxpr-capture-assertion-tests
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5686 +/- ##
==========================================
- Coverage 99.67% 99.67% -0.01%
==========================================
Files 414 414
Lines 39316 39167 -149
==========================================
- Hits 39188 39038 -150
- Misses 128 129 +1 ☔ View full report in Codecov by Sentry. |
Co-authored-by: David Wierichs <david.wierichs@xanadu.ai>
…nnyLaneAI/pennylane into plxpr-capture-assertion-tests
Context:
Following on from #5523, we need a good way of ensuring operations will integrate well with program capture.
Description of the Change:
This PR adds a check to the
assert_valid
function to ensure capture is working as expected.It then starts to fix up some operations that were failing to be captured correctly.
Benefits:
More robust integration with pl capture.
Possible Drawbacks:
I'm realizing a lot more operators fail to be immediately captured than I initially assumed 😢
Related GitHub Issues:
[sc-63310]