-
Notifications
You must be signed in to change notification settings - Fork 615
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
Shape error for tensor-like parameters in pauli arithmetic fixed (Addition to PR: #6562) #6587
Conversation
…thmetic + changelog
Co-authored-by: David Wierichs <[email protected]>
Co-authored-by: David Wierichs <[email protected]>
Co-authored-by: David Wierichs <[email protected]>
Co-authored-by: David Wierichs <[email protected]>
Co-authored-by: David Wierichs <[email protected]>
Co-authored-by: Christina Lee <[email protected]>
…tangle test cases
…tangle test cases
@dwierichs Should I close this PR? Without the pauli reps for parametrized gates in #6562 the Pauli arithmetic won't be used so there is no need to enable batched computation - or should the bug be corrected in case pauli reps for parametrized gates are added at a later stage? |
Thanks @ldi18 , |
Hi @ldi18 |
Hi @dwierichs and @astralcai, this is the updated PR but tests are still missing. The tests I created previously can't be used directly anymore as we didn't include the parametrized ops in PR #6562. We could overwrite some parametrized ops with class RX_with_pauli_rep(RX):
@property
def pauli_rep(self):
if self._pauli_rep is None:
self._pauli_rep = qml.pauli.PauliSentence(
{
qml.pauli.PauliWord({self.wires[0]: "I"}): qml.math.cos(self.data[0] / 2),
qml.pauli.PauliWord({self.wires[0]: "X"}): -1j * qml.math.sin(self.data[0] / 2),
}
)
return self._pauli_rep
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
class RY_with_pauli_rep(RY):
@property
def pauli_rep(self):
if self._pauli_rep is None:
self._pauli_rep = qml.pauli.PauliSentence(
{
qml.pauli.PauliWord({self.wires[0]: "I"}): qml.math.cos(self.data[0] / 2),
qml.pauli.PauliWord({self.wires[0]: "Y"}): -1j * qml.math.sin(self.data[0] / 2),
}
)
return self._pauli_rep
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs) in the test files. Then (RX_with_pauli_rep([1.5, 1, 1], wires=0) @ RY_with_pauli_rep([1.5, 1, 1], wires=0)).matrix() gives the expected shape error - and no error once the changes in |
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 @ldi18
As you already have that test with modified rotation gates, I think it would be great to include those!
Maybe in addition, could you add a (very minimal) test for the particular methods that are being modified? So, for example, something like
@pytest.mark.parametrize("coeff0", [np.array([0.6, 0.2, 4.3]), -0.7])
@pytest.mark.parametrize("coeff1", [np.array([1.2, -0.9, 2.7]), -0.7])
def test_to_mat_with_broadcasting(coeff0, coeff1):
wire_order = [0, 1, "a", "b"]
pw0 = qml.pauli.PauliWord({0: "X", "a": "Y"})
pw1 = qml.pauli.PauliWord({0: "Z", 1: "Y", "b": "Y"})
ps = qml.pauli.PauliSentence({pw1: coeff0, pw1: coeff1})
mat0 = ps.to_mat(wire_order=wire_order)
mat1 = qml.matrix(ps.operation(), wire_order=wire_order)
assert np.allclose(mat0, mat1)
Co-authored-by: David Wierichs <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6587 +/- ##
=======================================
Coverage 99.62% 99.62%
=======================================
Files 468 468
Lines 44144 44144
=======================================
Hits 43977 43977
Misses 167 167 ☔ View full report in Codecov by Sentry. |
@dwierichs That's a great suggestion and seems sufficient to test my changes. I didn't add my tests because they are very hypothetical and don't test the changes as directly your suggestion. Using arrays as input actually caused an ambiguous truth value in the
I implemented the tests with lists instead ([0.1, 0.1, 1] == 1 is False and not ambiguous). Probably the error for array input would surface again at some point, so I also added qml.math.all(coeff == 1) and a corresponding test to this PR. If you prefer to leave it out of this PR I can remove it of course. |
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.
LGTM! 🚀
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.
I'd second what @astralcai said about the changelog entry, and I like the additional fix in .operation()
💯
Thanks for your contribution @ldi18 🎉
…ition to PR: #6562) (#6587) **Context:** I added Pauli string decompositions for single qubit parametrized gates in PR: #6562. Pennylane exploits these Pauli string representations when, for example, computing ```python (qml.RX(1.11, wires=0) @ qml.RY(1.23, wires=0)).matrix() ``` The example above works as intended, but ```python (qml.RX([1.11, 2.11, 3.11], wires=0) @ qml.RY([1.23, 2.23, 3.23], wires=0)).matrix() # shape error ``` or ```python (qml.RX(np.array([1.11, 2.11, 3.11]), wires=0) @ qml.RY(np.array([1.23, 2.23, 3.23]), wires=0)).matrix() # shape error ``` returns a shape error. The code above should work since `RX`and `RY`support tensor-like (flattened) parameters as it can be seen from ```python qml.RX([1.11, 2.11, 3.11], wires=0).matrix() # works! ``` **Description of the Change:** I added an outer product in `pauli_arithmetic`. **Benefits:** - Pauli representations can now be leveraged for the matrix conversion of products of ops. - A Pauli representation of the `PhaseShift gate` could be added - this was not possible before as the `PCPhase` gate used the `pauli_arithmetic`, which gave an error in the pytests. **Possible Drawbacks:** None **Related GitHub Issues:** #6243 and PR: #6562 --------- Co-authored-by: Lasse Dierich <[email protected]> Co-authored-by: David Wierichs <[email protected]> Co-authored-by: Christina Lee <[email protected]>
Context:
I added Pauli string decompositions for single qubit parametrized gates in PR: #6562. Pennylane exploits these Pauli string representations when, for example, computing
The example above works as intended, but
or
returns a shape error. The code above should work since
RX
andRY
support tensor-like (flattened) parameters as it can be seen fromDescription of the Change:
I added an outer product in
pauli_arithmetic
.Benefits:
PhaseShift gate
could be added - this was not possible before as thePCPhase
gate used thepauli_arithmetic
, which gave an error in the pytests.Possible Drawbacks:
None
Related GitHub Issues:
#6243 and PR: #6562