Extend litinski transformation for all Pauli rotations#15974
Extend litinski transformation for all Pauli rotations#15974ShellyGarion wants to merge 48 commits into
Conversation
alexanderivrii
left a comment
There was a problem hiding this comment.
Thanks Shelly for starting this work. Though what we want is a bit more general that what you have right now: we would like to evolve an arbitrary (multi-qubit) Pauli under a Clifford, that is given LitinskiTransformation to handle circuits with PPR and PPM gates.
|
Thanks Shelly, I agree that we really-really need a good Rust structure for representing Paulis, and this is a really great start. It's still not completely clear to me how exactly you are planning to use it for extending Litinski transformation to handle general Pauli product rotations and measurements. For instance, are you thinking of refactoring pub struct PauliProductRotation {
pub pauli: Pauli,
pub angle: Param,
}
pub struct PauliProductMeasurement {
pub pauli: Pauli, // "neg" is incorporated into `pauli_phase`
}Though this specific suggestion is not perfect as it would require storing and reasoning about an unnecessary internal |
alexanderivrii
left a comment
There was a problem hiding this comment.
Thanks Shelly, I have left some more comments.
| for qbit in 0..(pauli_num_qubits) { | ||
| let out_pauli = Pauli { | ||
| pauli_z: out_z, | ||
| pauli_x: out_x, | ||
| pauli_phase: out_phase, | ||
| }; | ||
|
|
||
| // evolve the singe qubit pauli by cliff | ||
| let pz = pauli_z[qbit]; | ||
| let px = pauli_x[qbit]; | ||
| let (ev_p_sign, ev_p_z, ev_p_x, ev_p_indices) = | ||
| cliff.evolve_single_qubit_pauli(pz, px, qbit); | ||
|
|
||
| let evolved_pauli_phase = if ev_p_sign { 2 as u8 } else { 0 as u8 }; | ||
| // transform the evolved pauli to a dense format | ||
| let mut evolved_pauli_z = vec![false; pauli_num_qubits]; | ||
| let mut evolved_pauli_x = vec![false; pauli_num_qubits]; | ||
| for (&i, (&zv, &xv)) in ev_p_indices.iter().zip(ev_p_z.iter().zip(ev_p_x.iter())) { | ||
| evolved_pauli_z[i as usize] = zv; | ||
| evolved_pauli_x[i as usize] = xv; | ||
| } | ||
| // compose the ouput evolved dense paulies | ||
| let evolved_pauli = Pauli { | ||
| pauli_z: evolved_pauli_z, | ||
| pauli_x: evolved_pauli_x, | ||
| pauli_phase: evolved_pauli_phase, | ||
| }; |
There was a problem hiding this comment.
Logic-wise, this seems ok. But this does create a new Pauli object (with dense vectors z and x) a large number of times (pauli_num_qubits). I am wondering if you can improve performance of this code: by avoiding to conjugate by Is (as these do not change pauli), and by introducing a variant of evolve_pauli_by_clifford that modifies pauli in-place.
There was a problem hiding this comment.
I added a check that the single qubit Pauli isn't I (and some tests) in d744ad3.
There was a problem hiding this comment.
is the variant of evolve_pauli_by_clifford that modifies pauli in-place needed for this PR? we'll add PPM/PPR and consider this.
| /// Compose the Paulis p1 and p2. | ||
| /// Returns the output Pauli. | ||
| pub fn pauli_compose(p1: &Pauli, p2: &Pauli) -> Pauli { |
There was a problem hiding this comment.
Can you please clarify: do we first apply p1 and then p2, or the other way around?
There was a problem hiding this comment.
it's like p1.compose(p2) in the default python code:
https://quantum.cloud.ibm.com/docs/en/api/qiskit/qiskit.quantum_info.Pauli#compose
I also added a comment in d1a354a
| pub struct Pauli { | ||
| pub pauli_z: Vec<bool>, | ||
| pub pauli_x: Vec<bool>, | ||
| pub pauli_phase: u8, |
There was a problem hiding this comment.
I think after the offline discussion, we want this to be xz-phase and not the group phase.
| qct.global_phase = 0 | ||
|
|
||
| # The original circuit (written only using Clifford gates, PPR and PPM). | ||
| PPRZ = PauliProductRotationGate(Pauli("Z"), np.pi / 4) |
There was a problem hiding this comment.
Can we put this into a separate test and use only PPRs (except maybe for the CX)? It would be good to keep the original test in place as reference that the old paths still work as expected.
There was a problem hiding this comment.
The pass doesn't identify yet PPR gates (or RZ/RX/RY gates) with pi/2 angles as Cliffords. I'll add this capability and then update this test accordingly.
| use qiskit_circuit::{BlocksMode, Qubit, VarsMode}; | ||
|
|
||
| use super::remove_identity_equiv::average_gate_fidelity_below_tol; // ToDo: move a shared file? | ||
| use super::substitute_pi4_rotations::is_angle_close_to_multiple_of_pi_k; // ToDo: move a shared file? |
There was a problem hiding this comment.
shoule we move these functions (as well as is_ppr_angle_close_to_multiple_of_pi2 below to a shared place)?
perhaps unifying them?
and if so - where?
| "pauli_product_measurement", | ||
| ]; | ||
|
|
||
| const MINIMUM_TOL: f64 = 1e-12; // ToDo: add approxination_degree to the pass? |
There was a problem hiding this comment.
should we add an approxination_degree to the pass?
(to decide when a rotation gate is close enough to a pi/2 rotation, and then clifford)
alexanderivrii
left a comment
There was a problem hiding this comment.
Thanks for the great work, Shelly. Functionality-wise, this is shaping up quite well, I have left a few comments. I have not carefully looked at the dense_pauli code since we are planning to replace it by a more efficient code anyway, and I have not deeply looked at all of the tests yet.
The other question is: what benchmarks should we consider for measuring the performance of the new aspects of the LitinskiTransformation pass: identifying clifford-like rotations (especially PPRs) and commuting PPRs through Cliffords? Note that I am not suggesting to add these benchmarks to ASV as a part of this PR, but having some script to evaluate the pass performance is quite important.
| } | ||
| /// Modifies the tableau in-place by appending RZ-gate, | ||
| /// with an angle that is an integer multiple of pi/2 | ||
| pub fn append_rz(&mut self, qubit: usize, multiple: usize) { |
There was a problem hiding this comment.
Nitpick about the naming. Seeing the function append_rz defined for a Clifford class is confusing, as appending an RZ gate to a Clifford generally no longer results in a Clifford. Moreover, RZ gates correspond to rotation angles, not "multiples". Maybe you can call this append_multiple_of_s or append_clifford_rz. The same comment applies to other new functions in this file.
There was a problem hiding this comment.
we have a similar function in the python clifford class:
| 1 => self.append_s(qubit), | ||
| 2 => self.append_z(qubit), | ||
| 3 => self.append_sdg(qubit), | ||
| _ => unreachable!("RZ is only applicable for multiples of pi/2 rotations."), |
There was a problem hiding this comment.
If we have this function as a part of public API, we should be able to call it with any integer multiple of pi/2, not necessarily reduced modulo 4. The change is to match multiple.rem_euclid(4) instead. Again, this comment applies to other new functions in this file.
| (true, false) => {} // pauli Z on qubit | ||
| (true, true) => self.append_sx(indices[qubit] as usize), // pauli Y on qubit | ||
| (false, true) => self.append_h(indices[qubit] as usize), // pauli X on qubit | ||
| (false, false) => {} // pauli I on qubit (shouldn't get it since pauli is sparse) |
There was a problem hiding this comment.
I don't understand this comment that "pauli is sparse". You are defining a public function which accepts slices of z-values, x-values and indices. Nothing prevents a possible user of this function to pass paulis with I-terms, and actually I think this should be an acceptable behavior. If you really want to have a function that does not accept I-terms, I would like to see this reflected in the name and the docstring, and possibly have this function return some kind of an error rather than panicking or silently performing an incorrect computation.
There was a problem hiding this comment.
I'm not sure what is the best way to handle the "I" terms in this case, since it also affects the CX ladders.
| } | ||
| /// Modifies the tableau in-place by appending PPR gate, | ||
| /// with an angle that is an integer multiple of pi/2 | ||
| pub fn append_ppr( |
There was a problem hiding this comment.
Should we add some rust tests for append_ppr? Yes, I know that we are not doing this at the moment, but maybe it's a good time to start.
There was a problem hiding this comment.
Added a test here: 519cfba
(it still doesn't work for PPR with "I" terms, which I'm not sure what is the best way to handle)
There was a problem hiding this comment.
"I" terms are handled here (including tests): f6068d1
| use qiskit_circuit::{BlocksMode, Qubit, VarsMode}; | ||
|
|
||
| use super::remove_identity_equiv::average_gate_fidelity_below_tol; // ToDo: move a shared file? | ||
| use super::substitute_pi4_rotations::is_angle_close_to_multiple_of_pi_k; // ToDo: move a shared file? |
There was a problem hiding this comment.
I think we should eventually move these, but it does not have to be a part of this PR. In fact, let's leave this cleanup to a followup, since there are likely other common utility functions scattered throughout the code base.
There was a problem hiding this comment.
when we merge this, let's open an issue to track the code organization
| As well as the rotation gates above with angles that are integral multiples of :math:`pi/2` | ||
| (which are also Clifford). |
There was a problem hiding this comment.
As per your previous question, we should add the approximation_degree to the pass and slightly improve this comment (explaining the role of approximation degree when detecting Clifford rotations). Also you should mention PPRs here, right?
| In addition, rotation gates and :class:`.PauliProductRotationGates` with angles | ||
| that are integral multiples of :math:`pi/2` are treated as Clifford gates. |
There was a problem hiding this comment.
same comment as before that this should be related to the approximation degree.
| self.assertNotIn("rz", qc_litinski.count_ops()) | ||
| # self.assertNotIn("rz", qc_litinski.count_ops()) # ToDo: can we replace it by another check? |
There was a problem hiding this comment.
what is the reason for this?
There was a problem hiding this comment.
This is since after Litinski we still have some remaining RZ gates with pi/2 angles (which are Clifford)
There was a problem hiding this comment.
Any suggestion about how to update this test?
There was a problem hiding this comment.
I see, so now the Clifford-like RZ(pi/2) moves to the Clifford part of the circuit. I guess this is what we want (the alternative could be to synthesize this into explicit Clifford gates, but I don't like this either). Maybe then we can have two subtests: with fix_clifford=True we expect the operators to be equal, and with fix_clifford=False we expect to see no RZ gates remaining.
There was a problem hiding this comment.
that's a nice idea, I enhanced the QFT test here: 457dcc2
| def test_rotation_clifford_gates(self): | ||
| """Test circuit with rotation gates with multiple pi/2 angles which are clifford gates.""" |
There was a problem hiding this comment.
Do we also have a test with non-clifford rotations, clifford rotations and genuine cliffords?
There was a problem hiding this comment.
see 519cfba
(still without PPRs with "I" terms)
There was a problem hiding this comment.
"I" terms are handled here: f6068d1
and some tests are added
| # The transformed circuit (as shown at the bottom-right of the figure). | ||
| expected = QuantumCircuit(4, 4) | ||
| expected.append(PauliProductRotationGate(Pauli("Z"), np.pi / 4), [0]) | ||
| expected.append(PauliProductRotationGate(Pauli("YX"), np.pi / 4), [1, 2]) | ||
| expected.append(PauliProductRotationGate(Pauli("Y"), -np.pi / 4), [3]) | ||
| expected.append(PauliProductRotationGate(Pauli("YZZZ"), -np.pi / 4), [0, 1, 2, 3]) | ||
| expected.append(PauliProductMeasurement(Pauli("YZZY")), [0, 1, 2, 3], [0]) | ||
| expected.append(PauliProductMeasurement(Pauli("XX")), [0, 1], [1]) | ||
| expected.append(PauliProductMeasurement(Pauli("-Z")), [2], [2]) | ||
| expected.append(PauliProductMeasurement(Pauli("XX")), [0, 3], [3]) |
There was a problem hiding this comment.
In this and in other new tests, how do we know that the expected circuit is correct? Without measures/PPMs I would suggest to add comparing Operators.
There was a problem hiding this comment.
this is correct from the example given in Litinski's paper. We can't compare the Operators in this case due to the measures
|
After discussing with Shelly, a nice set of benchmarks would be to take the ASV benchmarks we already use for measuring the performance of |
Summary
Close #15862
Details and comments
In order to support PPR/PPM, this PR should include the following functions and methods:
clifford.evolve_single_qubit_pauli- evolve a single qubit pauli (X,Y,Z) by the given Cliffordpauli_compose(P1, P2)- a function that composes two n-qubit paulis P1 and P2evolve_pauli_by_clifford(P, C)- a function that uses the above in order to evolve an n-qubit pauli P by a clifford CThe functions above part were moved to #16137, and this PR should be based on #16137 (
DensePauliclass)This PR should therefore replace #14677
This PR should be based on #16137 (
DensePauliclass)In a follow up PR, we shall better organize and extend the functionality in rust of the
DensePauliandCliffordclasses.