Skip to content
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

Merged
merged 6 commits into from
Jul 2, 2024
Merged

Conversation

albi3ro
Copy link
Contributor

@albi3ro albi3ro commented Jun 28, 2024

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.

Screenshot 2024-06-28 at 5 38 15 PM

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:

Copy link

codecov bot commented Jun 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.66%. Comparing base (e74c9bf) to head (5f42bb8).
Report is 1 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@dwierichs dwierichs left a 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.

doc/releases/changelog-dev.md Show resolved Hide resolved
pennylane/tape/qscript.py Show resolved Hide resolved
pennylane/tape/qscript.py Show resolved Hide resolved
albi3ro and others added 2 commits July 2, 2024 09:47
Co-authored-by: David Wierichs <david.wierichs@xanadu.ai>
@albi3ro albi3ro requested a review from dwierichs July 2, 2024 13:48
Copy link
Contributor

@dwierichs dwierichs left a 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.

pennylane/tape/qscript.py Show resolved Hide resolved
tests/tape/test_qscript.py Outdated Show resolved Hide resolved
tests/tape/test_qscript.py Show resolved Hide resolved
tests/tape/test_qscript.py Outdated Show resolved Hide resolved
@lillian542 lillian542 self-requested a review July 2, 2024 15:15
@albi3ro albi3ro enabled auto-merge (squash) July 2, 2024 15:20
@albi3ro albi3ro merged commit 750a62f into master Jul 2, 2024
40 checks passed
@albi3ro albi3ro deleted the cache-qscript-hash branch July 2, 2024 15:38
RenkeHuang pushed a commit to RenkeHuang/pennylane that referenced this pull request Jul 11, 2024
**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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants