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

Add update kwarg to QuantumScript.copy #6285

Merged
merged 22 commits into from
Oct 1, 2024
Merged

Add update kwarg to QuantumScript.copy #6285

merged 22 commits into from
Oct 1, 2024

Conversation

lillian542
Copy link
Contributor

@lillian542 lillian542 commented Sep 19, 2024

Context:
We often make new, modified tapes in transforms, and occasionally an attribute of the old tape is not included. It can be unclear in some cases whether this was intentional or not. For example, in many transforms, we don't copy over trainable_params when making the new tape. It can be unclear whether this is because we are intending to set trainable_params to the default None, or whether it's an oversight. We've also had at least one transform where we missed copying shots over until later.

Description of the Change:
We add an update kwarg to the tape.copy method that takes a dict with possible keys operations, measurements, shots and/or trainable_params. Passing a dict to update changes the defined attributes while copying everything else over from the original tape.

Benefits:
If we use this in transforms, we won't be able to forget to copy over tape attributes - and if we overwrite anything, it will be clearly intentional (i.e. we will have to explicitly pass None to the update method instead of leaving the attribute out when creating a new tape).

[sc-65487]

This comment was marked as resolved.

@lillian542
Copy link
Contributor Author

[sc-65487]

@lillian542 lillian542 marked this pull request as ready for review September 19, 2024 20:42
@lillian542 lillian542 changed the title Tape update Add update method to QuantumScript Sep 19, 2024
Copy link

codecov bot commented Sep 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.70%. Comparing base (8f91b8a) to head (a44971c).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6285   +/-   ##
=======================================
  Coverage   99.70%   99.70%           
=======================================
  Files         444      444           
  Lines       42226    42234    +8     
=======================================
+ Hits        42103    42111    +8     
  Misses        123      123           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pennylane/tape/qscript.py Outdated Show resolved Hide resolved
pennylane/tape/qscript.py Outdated Show resolved Hide resolved
Copy link
Contributor

@PietropaoloFrisoni PietropaoloFrisoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this seems helpful!

doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
pennylane/tape/qscript.py Outdated Show resolved Hide resolved
pennylane/tape/qscript.py Outdated Show resolved Hide resolved
@lillian542 lillian542 changed the title Add update method to QuantumScript Add update kwarg to QuantumScript.copy Sep 20, 2024
pennylane/tape/qscript.py Outdated Show resolved Hide resolved
pennylane/tape/qscript.py Outdated Show resolved Hide resolved
Co-authored-by: Pietropaolo Frisoni <[email protected]>
Co-authored-by: Pietropaolo Frisoni <[email protected]>
Copy link
Contributor

@PietropaoloFrisoni PietropaoloFrisoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good to me!

I guess we just need to ensure that the latest doc version is nice and clean 👍

pennylane/tape/qscript.py Outdated Show resolved Hide resolved
pennylane/tape/qscript.py Outdated Show resolved Hide resolved
pennylane/tape/qscript.py Outdated Show resolved Hide resolved
pennylane/tape/qscript.py Outdated Show resolved Hide resolved
pennylane/tape/qscript.py Outdated Show resolved Hide resolved
pennylane/tape/qscript.py Outdated Show resolved Hide resolved
Copy link
Contributor

@astralcai astralcai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@lillian542 lillian542 enabled auto-merge (squash) October 1, 2024 16:02
@lillian542 lillian542 merged commit 0775d2f into master Oct 1, 2024
37 checks passed
@lillian542 lillian542 deleted the tape_update branch October 1, 2024 17:24
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.

4 participants