create read-only pauli_rep property #4017
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hello. You may have forgotten to update the changelog!
|
Codecov Report
@@ Coverage Diff @@
## master #4017 +/- ##
=======================================
Coverage 99.58% 99.58%
=======================================
Files 351 351
Lines 30832 30835 +3
=======================================
+ Hits 30703 30706 +3
Misses 129 129
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
converting to draft, can be used while addressing story 37494 |
timmysilv
added a commit
that referenced
this pull request
Dec 6, 2023
**Context:** The `pauli_rep` property is widely used, and should be made public **Description of the Change:** - Introduce the `pauli_rep` read-only property to the `Operator` class - Change all (read) references to `op._pauli_rep` into `op.pauli_rep` (the public version) **Benefits:** We're committing to having `pauli_rep` be part of the public API, making it less scary to lean on while moving forward with other improvements (eg. combining all the hamiltonian expansion transforms) **Possible Drawbacks:** We might want to change things related to it again, and it would require more careful work because it's public. I'm not so worried about this because it has existed (and been used) for a while now. **Related GitHub Issues:** previously attempted in #4017 [sc-37494] --------- Co-authored-by: Christina Lee <christina@xanadu.ai>
mudit2812
pushed a commit
that referenced
this pull request
Dec 7, 2023
**Context:** The `pauli_rep` property is widely used, and should be made public **Description of the Change:** - Introduce the `pauli_rep` read-only property to the `Operator` class - Change all (read) references to `op._pauli_rep` into `op.pauli_rep` (the public version) **Benefits:** We're committing to having `pauli_rep` be part of the public API, making it less scary to lean on while moving forward with other improvements (eg. combining all the hamiltonian expansion transforms) **Possible Drawbacks:** We might want to change things related to it again, and it would require more careful work because it's public. I'm not so worried about this because it has existed (and been used) for a while now. **Related GitHub Issues:** previously attempted in #4017 [sc-37494] --------- Co-authored-by: Christina Lee <christina@xanadu.ai>
mudit2812
pushed a commit
that referenced
this pull request
Jan 19, 2024
**Context:** The `pauli_rep` property is widely used, and should be made public **Description of the Change:** - Introduce the `pauli_rep` read-only property to the `Operator` class - Change all (read) references to `op._pauli_rep` into `op.pauli_rep` (the public version) **Benefits:** We're committing to having `pauli_rep` be part of the public API, making it less scary to lean on while moving forward with other improvements (eg. combining all the hamiltonian expansion transforms) **Possible Drawbacks:** We might want to change things related to it again, and it would require more careful work because it's public. I'm not so worried about this because it has existed (and been used) for a while now. **Related GitHub Issues:** previously attempted in #4017 [sc-37494] --------- Co-authored-by: Christina Lee <christina@xanadu.ai>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
op._pauli_repis now being read in various places, effectively making it part of theOperatorAPI. However, the attribute is private, which suggests otherwise. It seems we want it to stick around, so I'm making it a public (read-only) property.I'm not adding a setter because I don't want to encourage other places to set it. If you should, you know who you are (probably an arithmetic op, notice that every file changed in
pennylane/is inops/op_math😉 ) and you can use the private property. If it gets messy in the future, we can re-consider. But for now, I think this gives the best overall picture of the state of things.Suggested by Lee in this comment because I was using it in pennylane-lightning as if it was part of the Operator API.
UPDATE: I see @albi3ro you are hesitant to add more to the Operator API. I'd argue that this is effectively already part of it, so it should at least be codified. I agree that the Operator class is getting a touch bloated, but I'd prefer to address that as a separate concern. Perhaps we can make it a bit more of a priority