-
Notifications
You must be signed in to change notification settings - Fork 47
Implement a decomposition of PPRs into PPMs #1664
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
base: main
Are you sure you want to change the base?
Conversation
…asurements based on a boolean control. - Added convenience builders. - Improve the doc
Support PPR with negative angle Support Pass options for CLI
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
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 work!! :) So far I'm leaving some comments regarding organization, documentation, naming, etc. I will leave a second review regarding the C++ implementation tomorrow :)
Prepares logical qubits in a specific initial quantum state, | ||
such as |0⟩, |1⟩, |+⟩, |-⟩, |Y⟩, |-Y⟩, |m⟩, or |m̅⟩. | ||
|
||
This operation is typically used at the start of a quantum circuit to initialize |
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 wouldn't say this operation is used at the start of a circuit, don't we need this everywhere in order to implement pi/8 rotations?
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, we need this everywhere we decompose pi/8 rotations.
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.
One more thing, in Catalyst, we assume that the qubits are prepared in |0>. So if the program starts from |+>, the developer needs to apply the H gate when writing in Pennylane. In QEC, I think the preparation step and applying the gate could be different in terms of performance. So, I'm thinking of letting users define the prepared state as well.
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.
Do you plan on keeping this operation now that we have fabricate?
If yes, what are the semantics with respect to the input state? Does the prepare op require that the input is in a 0 state?
Co-authored-by: David Ittah <[email protected]>
Co-authored-by: David Ittah <[email protected]>
@@ -2,6 +2,20 @@ | |||
|
|||
<h3>New features since last release</h3> | |||
|
|||
* A new compilation pass has been added to Catalyst to decompose Pauli product rotations (PPRs) into | |||
Pauli product measurements (PPMs). Non-Clifford (pi/8) rotations require the consumption of a |
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.
Pauli product measurements (PPMs). Non-Clifford (pi/8) rotations require the consumption of a | |
Pauli product measurements (PPMs). Non-Clifford rotations, :math:`\exp(i\sigma_{x, y, y} \tfrac{\pi}{8})`, require the consumption of a |
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.
x, y, z?
The expression is also missing an indication that the exponential argument is a product.
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 don't know that these rendered forms add that much, unless we say we have to define the term PPR mathematically whenever we use it. It's also harder to read unrendered.
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 we should do something to clarify what the "pi/8" means, especially because, in PL, we take the convention of the factor of 1/2 being in the exponent already! I found that to be a pesky blocker when trying to roughly implement some things in the paper in PL.
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.
How about defining PPR once when it is first mentioned? Then you can say Non-Clifford (Θ=pi/8) rotations
assuming theta was the variable used in the definition
@@ -2,6 +2,20 @@ | |||
|
|||
<h3>New features since last release</h3> | |||
|
|||
* A new compilation pass has been added to Catalyst to decompose Pauli product rotations (PPRs) into | |||
Pauli product measurements (PPMs). Non-Clifford (pi/8) rotations require the consumption of a | |||
magic state, while Clifford rotations will not. The methods are implemented as described 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.
magic state, while Clifford rotations will not. The methods are implemented as described in | |
magic state, while Clifford rotations, :math:`\exp(i\sigma_{x, y, y} \tfrac{\pi}{4})`, will not. The methods are implemented as described in |
PPRs (pi/8) are decomposed first, and then Clifford PPRs (pi/4) are decomposed. Non-Clifford | ||
decomposition can be performed in one of two ways: "clifford-corrected" or "auto-corrected", |
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.
See changelog entry suggestion. I'd do the same here!
- Updated docstring - Renamed variable name
Co-authored-by: Isaac De Vlugt <[email protected]> Update doc/releases/changelog-dev.md Co-authored-by: Isaac De Vlugt <[email protected]>
clean format
- Applied `qec.fabricate` in DecomposeCliffordPPR - Improved code
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 work @sengthai 🎉 🎉
This is looking great, only a few points we could improve on :)
# | ||
# commute_ppr | ||
# |
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.
Here as well I would avoid these comments, you should use docstrings to document individual tests, and classes to group similar tests together. That's how we organize all our other pytests as well :)
In this case the headers don't really do much anyway since they just repeat the name of the test function.
|
||
def test_commute_ppr(): | ||
""" | ||
Test commute_ppr |
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 good docstring will say what you are testing and possibly additional information like what you are expecting (if that isn't obvious).
Prepares logical qubits in a specific initial quantum state, | ||
such as |0⟩, |1⟩, |+⟩, |-⟩, |Y⟩, |-Y⟩, |m⟩, or |m̅⟩. | ||
|
||
This operation is typically used at the start of a quantum circuit to initialize |
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.
Do you plan on keeping this operation now that we have fabricate?
If yes, what are the semantics with respect to the input state? Does the prepare op require that the input is in a 0 state?
I1:$condition, // The condition qubit | ||
PauliWord:$pauli_product, | ||
PauliWord:$else_pauli_product, |
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 would reorder these slightly, instead of framing it as an if-else with a condition, framing it as a switch (or selector) where the provided switch value simply indexes the provided pauli products makes more sense to me, because:
- it is easily extensible to more products with a larger integer
- it fits the
select.ppm
name better
I1:$condition, // The condition qubit | |
PauliWord:$pauli_product, | |
PauliWord:$else_pauli_product, | |
I1:$switch, | |
PauliWord:$pauli_product_0, | |
PauliWord:$pauli_product_1, |
/// ─────┌───┐─────── | ||
/// ─────│ P │(π/4)── | ||
/// ─────└───┘─────── | ||
/// | ||
/// into | ||
/// | ||
/// ─────┌───┐────────┌───────┐── | ||
/// ─────|-P |────────| P(π/2)|── | ||
/// ─────| |────────└───╦───┘── | ||
/// | ╠════════════╣ | ||
/// | | ┌───┐ ║ | ||
/// |0⟩──| Y |─────| X ╠══╝ | ||
/// └───┘ └───┘ |
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.
Wow these are nice drawings 😄
rewriter.create<PPRotationOp>(loc, pauliP, PI_DENOMINATOR, outPZQubits, cond.getResult()); | ||
|
||
rewriter.replaceOp(op, pprPI2.getOutQubits()); | ||
return pprPI2; |
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 we not destroy the ancilla before returning?
I was thinking it might be helpful to add single qubit allocation/deallocation ops to not have to deal with the register unless necessary, what do you think?
What I've also seen, which would work here, is a destructive measurement (that automatically deallocates the qubit).
namespace catalyst { | ||
namespace qec { | ||
|
||
AllocOp buildAllocQreg(Location loc, int64_t size, PatternRewriter &rewriter) |
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 looks like these should be builders on the operations directly no?
return rewriter.create<ExtractOp>(loc, type, qreg, nullptr, idxAttr); | ||
} | ||
|
||
LogicalInitKind getMagicState(QECOpInterface op) |
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 this method be part of the interface then?
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.
Generally speaking, "utility" modules often hint at insufficient or poorly designed apis. Not that they never have their place, but like in this case, the functionality is often better placed directly with the relevant code constructs (classes, modules, etc).
auto loc = op.getLoc(); | ||
|
||
// Prepare |0⟩ and |m⟩ | ||
auto zero = rewriter.create<FabricateOp>(loc, LogicalInitKind::zero); |
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.
Similar from the other file, it might be nice to distinguish between using a factory vs regular ancilla allocation.
And same comments regarding the deallocation of ancilla qubits.
Context:
This PR introduces a new pass that decomposes Pauli Product Rotations (PPR) into Pauli Product Measurements (PPMs) consuming magic states. Two decomposition strategies are supported: inject-magic-state and auto-corrected. Related prior work on qec.ppr began in #1486 #1563 #1580.
Description of the Change:
qec.select.ppm
– conditional Pauli product measurement based on a boolean controlqec.prepare
– prepares logical qubits in a specified initial state (|0⟩, |1⟩, |+⟩, |-⟩, |Y⟩, |-Y⟩, |m⟩, or |m̅⟩)qec.ppr
andqec.ppm
to support conditional executioninject-magic-state
andauto-corrected
.Example:
Input:
Run:
$ quantum-opt --decompose_clifford_ppr='decompose-method=auto-corrected' test.mlir
Outputs:
Run:
$ quantum-opt --decompose_clifford_ppr='decompose-method=inject-magic-state' test.mlir
Outputs:
[sc-89168]
[sc-90158]