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 101215 +258
========================================
+ Hits 100557 100815 +258
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
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
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
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!
|
|
||
| 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.
There was a problem hiding this comment.
pylint checks if the error is documented in the Raise section:
W9006: "AssertionError" not documented as being raised (missing-raises-doc)
The suppression is working at the function level, so it also suppresses checks for other exceptions in this method as well (I can't only suppress the AssertionError):
# This works:
def sparse_matrix(self, qubits) -> sparse.csr_matrix:
# pylint: disable=missing-raises-doc
# This failed:
raise AssertionError("...") #pylint: disable=missing-raises-doc
There was a problem hiding this comment.
Sounds good, let us leave it as is. Thank you for checking this.
...and also add annotations, checking values by np.array_equal.
4 participants
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