-
Notifications
You must be signed in to change notification settings - Fork 604
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
Transform for Clifford+T decomposition #4655
Conversation
…nnylane into clifford-t-decomp
Hello. You may have forgotten to update the changelog!
|
[sc-43354] |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4655 +/- ##
==========================================
- Coverage 99.64% 99.63% -0.01%
==========================================
Files 380 382 +2
Lines 34275 34260 -15
==========================================
- Hits 34154 34136 -18
- Misses 121 124 +3
☔ View full report in Codecov by Sentry. |
pennylane/transforms/decompositions/clifford_plus_t/cliffordt_transform.py
Outdated
Show resolved
Hide resolved
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 @obliviateandsurrender! Let me know if I can help clarify any of the comments.
pennylane/transforms/decompositions/clifford_plus_t/cliffordt_transform.py
Outdated
Show resolved
Hide resolved
pennylane/transforms/decompositions/clifford_plus_t/cliffordt_transform.py
Show resolved
Hide resolved
pennylane/transforms/decompositions/clifford_plus_t/cliffordt_transform.py
Show resolved
Hide resolved
pennylane/transforms/decompositions/clifford_plus_t/cliffordt_transform.py
Outdated
Show resolved
Hide resolved
pennylane/transforms/decompositions/clifford_plus_t/cliffordt_transform.py
Show resolved
Hide resolved
pennylane/transforms/decompositions/clifford_plus_t/cliffordt_transform.py
Outdated
Show resolved
Hide resolved
pennylane/transforms/decompositions/clifford_plus_t/cliffordt_transform.py
Outdated
Show resolved
Hide resolved
pennylane/transforms/decompositions/clifford_plus_t/cliffordt_transform.py
Outdated
Show resolved
Hide resolved
pennylane/transforms/decompositions/clifford_plus_t/cliffordt_transform.py
Outdated
Show resolved
Hide resolved
pennylane/transforms/decompositions/clifford_plus_t/cliffordt_transform.py
Show resolved
Hide resolved
…o clifford+t_tranform
…o clifford+t_tranform
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.
Reviewed docs. Will look at code now.
pennylane/transforms/decompositions/clifford_plus_t/cliffordt_transform.py
Outdated
Show resolved
Hide resolved
pennylane/transforms/decompositions/clifford_plus_t/cliffordt_transform.py
Outdated
Show resolved
Hide resolved
pennylane/transforms/decompositions/clifford_plus_t/cliffordt_transform.py
Outdated
Show resolved
Hide resolved
pennylane/transforms/decompositions/clifford_plus_t/cliffordt_transform.py
Show resolved
Hide resolved
|
||
Args: | ||
qfunc (function): A quantum function | ||
max_depth (int): The depth to use for tape expansion before manual decomposition to Clifford+T basis is applied |
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.
max_depth (int): The depth to use for tape expansion before manual decomposition to Clifford+T basis is applied | |
max_depth (int): How many times the tape is expanded before manual decomposition to Clifford+T basis is applied |
Also, what does manual decomposition mean here?
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.
It means the actual decomposition to the Clifford+T
basis.
Keyword Args: | ||
Arguments (*): |
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 looks kind of strange in the preview. Could it just say "Keyword Args:" and not include "Arguments (*):"?
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 am doing so to escape making a big list of kwargs
for individual items. I couldn't find another way to do it.
Args: | ||
qfunc (function): A quantum function | ||
max_depth (int): The depth to use for tape expansion before manual decomposition to Clifford+T basis is applied | ||
method (str): Method to be used for Clifford+T decomposition. Default value is ``"sk"`` for Solovay-Kitaev |
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.
Solovay-Kitaev is the only method for now, but planning to add the other one later?
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, at least I hope so!
ValueError: If any gate operation does not have any existing rule for its decomposition | ||
NotImplementedError: If chosen decomposition is not supported | ||
|
||
.. seealso:: :func:`~.sk_decomposition` and :func:`~.sk_approximate_set` |
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.
Can we also have an example? 🙏
pennylane/transforms/decompositions/clifford_plus_t/cliffordt_transform.py
Outdated
Show resolved
Hide resolved
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.
mostly looks good! i have a weird feeling that we're missing some testing to be sure it's totally ready but it's not coming to me
next_gate_idx = find_next_gate(curr_gate.wires, copied_ops[1:]) | ||
|
||
# Replace the current gate, add it to merged list and pop it from orginal one | ||
merged_ops.append(curr_gate.__class__(*cumulative_angles, wires=curr_gate.wires)) |
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 probably better for this use-case, but in general you may want to use bind_new_parameters
pennylane/transforms/decompositions/clifford_plus_t/cliffordt_transform.py
Show resolved
Hide resolved
pennylane/transforms/decompositions/clifford_plus_t/cliffordt_transform.py
Outdated
Show resolved
Hide resolved
pennylane/transforms/decompositions/clifford_plus_t/cliffordt_transform.py
Outdated
Show resolved
Hide resolved
|
||
matrix_op = reduce( | ||
lambda x, y: x @ y, [qml.matrix(op) for op in decomp_ops][::-1] | ||
) * qml.matrix(global_ops) |
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.
should it not be this?
) * qml.matrix(global_ops) | |
) @ qml.matrix(global_ops) |
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.
The matrix for GlobalPhase
is like this ([[phase]])
, so I think the latter would throw an error.
Context: Adds a transform to perform Clifford+T decomposition
Description of the Change: Unrolls the circuit into Clifford+Rotation basis and then uses the functionality to obtain approximations for arbitrary z-rotations up to arbitrary ϵ for the operations.
Benefits: Allows one to transform any random circuit to a Clifford+Phase basis.
Possible Drawbacks: Might be sub-optimal
Related GitHub Issues: