-
Notifications
You must be signed in to change notification settings - Fork 603
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
Adds elementary gate decomposition for single-qubit QubitUnitary
#1427
Conversation
Hello. You may have forgotten to update the changelog!
|
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.
@glassnotes this is awesome! A couple of things that crossed my mind:
-
This means that we can now support
qml.Hermitian
(arbitrary Hermitian matrix observables) directly on hardware, since it currently diagonalizes down to aQubitUnitary
rotation 🙌 -
I love that it results in differentiability 🤯
-
In your code block in the PR comment, I don't think you need the
op.queue()
? Calling.decomposition
should automatically queue the ops, so they might be being queued twice. -
Part of me wonder if this feels like a compilation transform? Becuase there will be use cases where users want to force a unitary decomposition on a device that natively supports it. But I don't know? it would add overhead compared to simply calling
QubitUnitary.decomposition()
manually. -
Don't forget to add interface tests! To ensure it works with different tensor input
-
Along the same lines, gradient tests should also be added 🙂
pennylane/ops/qubit.py
Outdated
# if desired. If the top left element is 0, can only use the off-diagonal elements | ||
if qml.math.isclose(U[0, 0], 0): | ||
phi = 1j * qml.math.log(U[0, 1] / U[1, 0]) | ||
omega = -phi - 2 * qml.math.angle(U[1, 0]) |
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.
You may need to be careful here:
phi
is complex2 * angle(...)
will be real (I thinkfloat64
)
and TensorFlow (maybe also PyTorch?) will not automatically cast the latter to a complex number before doing the subtraction.
I think the following should work:
omega = -phi - 2 * qml.math.angle(U[1, 0]) | |
omega = -phi - qml.math.cast_like(2 * qml.math.angle(U[1, 0]), phi) |
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.
(Note: qml.math.cast(..., "complex128")
would also work above, but cast_like
is safer, in that it will support both complex64
and complex128
depending on whatever phi
is)
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.
Oh thanks, yes this looks safer! I originally had the conversion to real
in the line of phi
rather than in the return value, do you think it'd be better to convert phi
first so that the subtraction ends up real? Or is it better to just do the casting anyways?
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 think you'd need the casting regardless, potentially you could have float64 subtract float32, which would also cause an error
pennylane/ops/qubit.py
Outdated
omega = -phi - 2 * qml.math.angle(U[1, 0]) | ||
else: | ||
omega = 1j * qml.math.log(qml.math.tan(theta / 2) * U[0, 0] / U[1, 0]) | ||
phi = -omega - 2 * qml.math.angle(U[0, 0]) |
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.
phi = -omega - 2 * qml.math.angle(U[0, 0]) | |
phi = -omega - qml.math.cast_like(2 * qml.math.angle(U[0, 0]), omega) |
Co-authored-by: Josh Izaac <josh146@gmail.com>
I am wondering this too; so, for additional context, I'd like to do this for the 2-qubit case as well. That's going to make the |
Oh! Maybe the answer is... both?
|
I like it! I'll give it a go and we can see what it looks like. After deciding on that I'll make the rest of the tests. |
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 really like the new organization! Especially the fact that the decompositions can be done three ways:
- Automatically if the device does not support it
- Manually by calling
qml.QubitUnitary.decomposition(U)
- Manually on an existing qfunc via a transform
pennylane/ops/qubit.py
Outdated
@staticmethod | ||
def decomposition(U, wires): | ||
# Decompose arbitrary single-qubit unitaries as the form RZ RY RZ | ||
if qml.math.shape(U)[0] == 2: | ||
wire = Wires(wires)[0] | ||
decomp_ops = qml.transforms.decompositions._zyz_decomposition(U, wire) | ||
return decomp_ops | ||
|
||
return NotImplementedError("Decompositions only supported for single-qubit unitaries") |
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.
This is really nice! In particular, I like how the logic is in the transforms
package in its own module. This will make it much easier to modify and test for new contributors, rather than looking through the already large qubit.py
file.
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.
Yes, thanks for the suggestion to separate it out! 😁 It will also make it much cleaner to add different decompositions, one for 2-qubit unitaries, etc. while keeping the qubit ops file tidy.
@@ -0,0 +1,114 @@ | |||
# Copyright 2018-2021 Xanadu Quantum Technologies Inc. |
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.
Suggest renaming this file to better reflect the contents? Unless you envision multiple decompositions living here.
In the latter case, we could even have transforms/decompositions/single_qubit_unitary.py
?
""" | ||
# Check dimensions | ||
if qml.math.shape(U)[0] != 2 or qml.math.shape(U)[1] != 2: | ||
raise ValueError("Cannot convert matrix with shape {qml.math.shape(U)} to SU(2).") |
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.
raise ValueError("Cannot convert matrix with shape {qml.math.shape(U)} to SU(2).") | |
raise ValueError(f"Cannot convert matrix with shape {qml.math.shape(U)} to SU(2).") |
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.
Minor, but might be worth saving this value once to avoid it being recomputed 3 times:
shape = qml.math.shape(U)
if shape != (2, 2):
raise ValueError(f"Cannot convert matrix with shape {shape} to SU(2).")
omega = 2 * qml.math.angle(U[1, 1]) | ||
return [qml.RZ(omega, wires=wire)] |
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.
Nice!
cos2_theta_over_2 = qml.math.abs(U[0, 0] * U[1, 1]) | ||
theta = 2 * qml.math.arccos(qml.math.sqrt(cos2_theta_over_2)) |
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.
very very very minor, but you could have at the top of the file
from pennylane import math
which would then allow you to have
cos2_theta_over_2 = math.abs(U[0, 0] * U[1, 1])
theta = 2 * math.arccos(math.sqrt(cos2_theta_over_2))
which might be slightly nicer for readability?
dim_U = qml.math.shape(op.parameters[0])[0] | ||
|
||
if dim_U != 2: | ||
continue |
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.
Since the unitary size is already taken into account here, it would be nice to disable the then redundant check in _zyz_decomposition
🤔
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.
Actually I think the checks here are the ones to disable since this checked in _zyz_decomposition
via _convert_to_su2
. I think the check needs to stay in there because that's what gets called by the QubitUnitary.decomposition
method (which itself doesn't do a full check for unitarity).
Never mind this check should not be disabled. Which check were you referring to?
original_grad = qml.grad(original_qnode)(input) | ||
transformed_grad = qml.grad(transformed_qnode)(input) | ||
|
||
assert qml.math.allclose(original_grad, transformed_grad) |
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.
💪
|
||
original_grad = jax.grad(original_qnode)(input) | ||
transformed_grad = jax.grad(transformed_qnode)(input) | ||
assert qml.math.allclose(original_grad, transformed_grad, atol=1e-7) |
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.
amazing testing 💯
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.
All my comments are minor, as soon as the failing tests are fixed, I'm happy to approve!
|
||
|
||
@qfunc_transform | ||
def decompose_single_qubit_unitaries(tape): |
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.
You could even have
decompose_single_qubit_unitaries(tape, decomp=zyz_decomposition)
which would allow users to swap out their own decompositions if they want
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.
A separate thought: I don't really like the name, but am going to be really annoying when I say I don't have any better suggestions 🤔
- I find it a bit too long
- I keep remembering that the compile PR uses 'expand' instead of 'decompose'
You could have @decompose_unitaries
, and then have it as an implementation detail that only single qubit unitaries are supported, but that also doesn't feel right.
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.
decompose_single_qubit_unitaries(tape, decomp=zyz_decomposition)
Oh I like this idea!
A separate thought: I don't really like the name, but am going to be really annoying when I say I don't have any better suggestions
I find it a bit too long
I keep remembering that the compile PR uses 'expand' instead of 'decompose'
Yes I feel the same (about this and some of the other compilation transforms). Some other ideas:
decompose_qubit_unitaries
unitaries_to_rot
/unitary_to_rot
qubit_unitary_to_rot
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.
Oh I like unitary_to_rot
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.
Me too 👍 It's the shortest and also captures that it's for single-qubit operations 🎉
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.
decompose_single_qubit_unitaries(tape, decomp=zyz_decomposition)
Actually, if we are going to have the transform convert things to Rot
, it makes sense to keep as-is rather than adding the option, since Rot
is intrinsically zyz
. But later if we add more decompositions and start returning them as RZ, RY, RZ
, etc. instead of Rot
, this would make things more flexible.
>>> dev = qml.device('default.qubit', wires=1) | ||
>>> qnode = qml.QNode(qfunc, dev) | ||
>>> print(qml.draw(qnode)()) | ||
0: ──U0──┤ ⟨Z⟩ | ||
U0 = | ||
[[-0.17111489+0.58564875j -0.69352236-0.38309524j] | ||
[ 0.25053735+0.75164238j 0.60700543-0.06171855j]] | ||
|
||
We can use the transform to decompose the gate: | ||
|
||
>>> transformed_qfunc = decompose_single_qubit_unitaries(qfunc) | ||
>>> transformed_qnode = qml.QNode(transformed_qfunc, dev) | ||
>>> print(qml.draw(transformed_qnode)()) | ||
0: ──Rot(-1.35, 1.83, -0.606)──┤ ⟨Z⟩ |
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.
This is great!
@@ -0,0 +1,86 @@ | |||
# Copyright 2018-2021 Xanadu Quantum Technologies Inc. |
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.
Very minor: What do you think about this transform also living in the decompositions/
directory? I'm on the fence, but curious to get your thoughts.
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'm inclined to leave it where it is to keep the actual transform (which could be used in a compilation pipeline) separate from the different decompositions, which are not transforms and have multiple use cases.
r"""This module contains decompositions for (numerically-specified) arbitrary | ||
unitary operations into sequences of elementary operations. |
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 can imagine this being really useful going forward! Both for devs, and external contributors that want to contribute new decompositions to PL without the overhead of having to learn the Operations
class.
Co-authored-by: Josh Izaac <josh146@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #1427 +/- ##
==========================================
- Coverage 98.24% 98.23% -0.01%
==========================================
Files 160 163 +3
Lines 12027 12076 +49
==========================================
+ Hits 11816 11863 +47
- Misses 211 213 +2
Continue to review full report at Codecov.
|
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.
This functionality looks great @glassnotes, very happy to approve! In particular, I like how you separated the tests into tests that only check interface decomposition, and tests that only check interface gradients. It is very tempting when writing tests to combine them into a single test (which I have done a lot in the codebase).
Just two minor comments:
zyz_decomposition
doesn't currently appear in the docs- I'm not sure how important this is, but it could be nice to parametrize the gradient tests with
diff_method = ["parameter-shift", "backprop"]
, just to make sure everything works correctly with the non-defaultparameter-shift
logic.
phi = 1j * math.log(U[0, 1] / U[1, 0]) | ||
omega = -phi - math.cast_like(2 * math.angle(U[1, 0]), phi) | ||
else: | ||
el_division = U[0, 0] / U[1, 0] |
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.
El Division sounds like a math-parody mariachi band
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/pennylane into su2_su4_decomposition
decomp_ops = qml.transforms.decompositions.zyz_decomposition(U, wire) | ||
return decomp_ops | ||
|
||
raise NotImplementedError("Decompositions only supported for single-qubit unitaries") |
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.
Not sure why this isn't getting caught; the corresponding test is on line 1593 of the test_qubit_ops
file.
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.
Ah, I think this is a well-known coverage bug. Fine to ignore.
Context: Currently, there is no decomposition/synthesis for the arbitrary
QubitUnitary
operation.Description of the Change: Adds the math needed to express arbitrary single-qubit matrices as a
Rot
gate (or anRZ
gate, if they are diagonal).Benefits: This should allow
QubitUnitary
to run on more devices. Furthermore, I think that by using the decomposition we can makeQubitUnitary
fully differentiable, at least for the single-qubit case (see example below).Possible Drawbacks: None
Related GitHub Issues: None
Example: