-
Notifications
You must be signed in to change notification settings - Fork 585
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
turn QuantumScript.hash into a cached property #5919
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5919 +/- ##
==========================================
- Coverage 99.67% 99.66% -0.01%
==========================================
Files 425 425
Lines 40772 40479 -293
==========================================
- Hits 40640 40345 -295
- Misses 132 134 +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.
Nice and simple upgrade @albi3ro ! 🎉
I think it might be good to update and/or extend the tests to check the caching and to test that the hash is not maintained between unequal tapes created via _update_...
calls.
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.
Thanks for updating the test.
Had some minor suggestions, but feel free to discard them.
Co-authored-by: David Wierichs <david.wierichs@xanadu.ai>
**Context:** When investigating some performance issues, I noticed that we weren't caching `QuantumScript.hash`, even though calculating it is a known performance bottleneck. **Description of the Change:** Make `QuantumScript.hash` a cached property. **Benefits:** Much lower classical overhead when we are using caching. <img width="839" alt="Screenshot 2024-06-28 at 5 38 15 PM" src="https://github.com/PennyLaneAI/pennylane/assets/6364575/9d25401d-af74-455b-8a18-04fc7dc07c33"> **Possible Drawbacks:** If someone does bad things and mutates the tape in place, they will have to deal with the resulting consequences. **Related GitHub Issues:** --------- Co-authored-by: David Wierichs <david.wierichs@xanadu.ai>
Context:
When investigating some performance issues, I noticed that we weren't caching
QuantumScript.hash
, even though calculating it is a known performance bottleneck.Description of the Change:
Make
QuantumScript.hash
a cached property.Benefits:
Much lower classical overhead when we are using caching.
Possible Drawbacks:
If someone does bad things and mutates the tape in place, they will have to deal with the resulting consequences.
Related GitHub Issues: