Add sparse_matrix() to PauliString and PauliSum#8127
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8127 +/- ##
========================================
Coverage 99.60% 99.60%
========================================
Files 1118 1118
Lines 100957 101112 +155
========================================
+ Hits 100557 100712 +155
Misses 400 400 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
For each PauliString term, row/col indices and phases are computed directly via bitwise ops on basis states to avoid Kron product. For PauliSum, uses COO triplet accumulation instead of repeated CSR addition to avoid merging multiple sparse matrices.
|
@ToastCheng Thank you for this work! |
mhucka
left a comment
There was a problem hiding this comment.
Thank you for working on this!
I have some very small requests; otherwise, it looks good to me. I think @pavoljuhas had better review this too.
In addition, update implementation details: - use np.where parity check for phases - iterate self.items() directly
ee039de to
e4e84ae
Compare
|
@ToastCheng Thanks again for this contribution. Apart from agreeing with Pavol's point above, about adding an asserion, it's good to go from my perspective. |
|
Thank you @mhucka. I asked @dstrain115 for a one quick look, so let's allow some time for that. Otherwise this looks good to me too. |
dstrain115
left a comment
There was a problem hiding this comment.
Would recommend tightening up the test for pauli sum sparse matrix a bit, but otherwise I approve.
| assert np.allclose(H3, paulisum.matrix([q[1], q[2], q[0]])) | ||
|
|
||
|
|
||
| def test_pauli_sum_sparse_matrix() -> None: |
There was a problem hiding this comment.
I would recommend parameterizing this test case and passing in a variety of pauli sums.
There was a problem hiding this comment.
@dstrain115 when you get a chance, would you review & approve the PR if it's ready?
bb1956c to
a4faba8
Compare
pavoljuhas
left a comment
There was a problem hiding this comment.
I have a couple of minor suggestions, otherwise this looks great. Thank you @ToastCheng for contributing this!
| (cirq.X(q0) * cirq.I(q1) + cirq.Z(q1), None), | ||
| ), | ||
| ) | ||
| def test_pauli_sum_sparse_matrix(paulisum, qubits) -> None: |
There was a problem hiding this comment.
Nit - let us annotate the argument types here:
def test_pauli_sum_sparse_matrix(paulisum: cirq.PauliSum, qubits: list[cirq.Qid] | None) -> None:
| def test_pauli_sum_sparse_matrix(paulisum, qubits) -> None: | ||
| actual = paulisum.sparse_matrix(qubits).toarray() | ||
| expected = paulisum.matrix(qubits) | ||
| assert np.allclose(actual, expected) |
There was a problem hiding this comment.
Nit - we should use np.array_equal here to ensure the shapes of compared arrays are the same. (All tested coefficients have exact binary representation so there should be no problem with roundoffs)
| def test_pauli_sum_sparse_matrix_empty() -> None: | ||
| q = cirq.LineQubit.range(2) | ||
| empty = cirq.PauliSum.from_pauli_strings([]) | ||
| assert np.allclose(empty.sparse_matrix(q).toarray(), np.zeros((4, 4))) |
There was a problem hiding this comment.
We should use array_equal as above and we should also check the default sparse_matrix:
assert np.array_equal(empty.sparse_matrix().toarray(), np.zeros((1, 1)))
assert np.array_equal(empty.sparse_matrix(q).toarray(), np.zeros((4, 4)))|
|
||
| Raises: | ||
| NotImplementedError: If this `PauliString` is parameterized. | ||
| AssertionError: If an unexpected Pauli gate instance is encountered. |
There was a problem hiding this comment.
Nit - perhaps we should leave AssertionError from the docstring. Assertions are supposed to be always true regardless of user input; they should only pop up if there is a breaking change in the code or some unexpected code pathway.
For each PauliString term, row/col indices and phases are computed directly via bitwise ops on basis states to avoid Kron product.
For PauliSum, uses COO triplet accumulation instead of repeated CSR addition to avoid merging multiple sparse matrices.
Fixes #3057