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

Make Operator.data a tuple instead of a list #4222

Merged
merged 16 commits into from Jun 15, 2023
Merged

Make Operator.data a tuple instead of a list #4222

merged 16 commits into from Jun 15, 2023

Conversation

eddddddy
Copy link
Collaborator

@eddddddy eddddddy commented Jun 6, 2023

Context:
Part of the ongoing effort towards operator immutability.

Description of the Change:
Make Operator.data a tuple instead of a list. Change everywhere Operator.data is used to expect a tuple instead of a list.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@codecov
Copy link

codecov bot commented Jun 7, 2023

Codecov Report

Merging #4222 (8dcca47) into master (1c5e44b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4222   +/-   ##
=======================================
  Coverage   99.78%   99.78%           
=======================================
  Files         352      352           
  Lines       31741    31754   +13     
=======================================
+ Hits        31673    31686   +13     
  Misses         68       68           
Impacted Files Coverage Δ
pennylane/ops/op_math/adjoint.py 100.00% <ø> (ø)
pennylane/ops/op_math/controlled.py 100.00% <ø> (ø)
pennylane/operation.py 97.33% <100.00%> (+0.01%) ⬆️
pennylane/ops/op_math/composite.py 100.00% <100.00%> (ø)
pennylane/ops/op_math/evolution.py 100.00% <100.00%> (ø)
pennylane/ops/op_math/symbolicop.py 100.00% <100.00%> (ø)
pennylane/ops/qubit/hamiltonian.py 100.00% <100.00%> (ø)
pennylane/optimize/adaptive.py 100.00% <100.00%> (ø)
pennylane/tape/qscript.py 99.04% <100.00%> (+0.01%) ⬆️

@eddddddy eddddddy marked this pull request as ready for review June 7, 2023 19:31
@eddddddy eddddddy requested review from mudit2812 and a team June 7, 2023 20:18
Copy link
Contributor

@mudit2812 mudit2812 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 the changes @eddddddy ! I'm hesitant about the changes to QuantumScript._par_info, but apart from that, this PR looks good :)

pennylane/operation.py Outdated Show resolved Hide resolved
pennylane/tape/qscript.py Outdated Show resolved Hide resolved
pennylane/tape/qscript.py Outdated Show resolved Hide resolved
tests/legacy/test_hamiltonian_expand_old.py Show resolved Hide resolved
tests/ops/op_math/test_composite.py Outdated Show resolved Hide resolved
tests/transforms/test_hamiltonian_expand.py Outdated Show resolved Hide resolved
tests/legacy/test_qscript_old.py Outdated Show resolved Hide resolved
tests/legacy/test_qscript_old.py Outdated Show resolved Hide resolved
tests/tape/test_qscript.py Outdated Show resolved Hide resolved
tests/tape/test_qscript.py Outdated Show resolved Hide resolved
Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
Copy link
Contributor

@timmysilv timmysilv left a comment

Choose a reason for hiding this comment

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

are we going to remove data setters soon? I was going to ask if we should update the setters in various files (evolution.py, pow.py and symbolicop.py) to cast the inputted new_data as tuples before setting it, but I realize we'd probably rather just delete the setters altogether!

Keen to approve, but very hesitant about the _par_info/get_operation change. I don't want to block this PR because a problem with QuantumScript, but perhaps a bit more clarity on direction first

pennylane/operation.py Show resolved Hide resolved
@mudit2812
Copy link
Contributor

are we going to remove data setters soon? I was going to ask if we should update the setters in various files (evolution.py, pow.py and symbolicop.py) to cast the inputted new_data as tuples before setting it, but I realize we'd probably rather just delete the setters altogether!

The intention is to start using bind_new_parameters to replace the use of data setters and entirely move away from in-place mutation of operators. Changes towards this are already being made (eg. #4220)

@eddddddy
Copy link
Collaborator Author

eddddddy commented Jun 9, 2023

Do we want to remove the setters in this PR or a follow-up one?

@mudit2812
Copy link
Contributor

Do we want to remove the setters in this PR or a follow-up one?

@albi3ro can confirm, but I'd imagine a follow-up one. It's a pretty major breaking change so it would likely need a deprecation cycle.

@timmysilv
Copy link
Contributor

definitely don't remove in this PR - if there was anything to do in this one, it would be to add casting to tuple in existing data setters, but I'm ok to just leave them be. tl;dr nothing to do here 😄

Copy link
Contributor

@timmysilv timmysilv left a comment

Choose a reason for hiding this comment

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

last comment, otherwise lgtm and I will approve:)

pennylane/tape/qscript.py Show resolved Hide resolved
pennylane/tape/qscript.py Show resolved Hide resolved
@timmysilv timmysilv added this to the v0.31 milestone Jun 12, 2023
Copy link
Contributor

@mudit2812 mudit2812 left a comment

Choose a reason for hiding this comment

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

Thanks @eddddddy !

pennylane/operation.py Outdated Show resolved Hide resolved
eddddddy and others added 2 commits June 14, 2023 21:48
Co-authored-by: Christina Lee <christina@xanadu.ai>
@eddddddy
Copy link
Collaborator Author

[sc-36757]

@eddddddy eddddddy merged commit 58bdaea into master Jun 15, 2023
43 checks passed
@eddddddy eddddddy deleted the op_data_tuple branch June 15, 2023 18:45
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.

None yet

4 participants