-
Notifications
You must be signed in to change notification settings - Fork 61
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
pauli refactor #539
pauli refactor #539
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #539 +/- ##
======================================
Coverage 98.1% 98.1%
======================================
Files 160 162 +2
Lines 3084 3112 +28
Branches 750 760 +10
======================================
+ Hits 3026 3054 +28
Misses 37 37
Partials 21 21 ☔ View full report in Codecov by Sentry. |
toqito/matrices/pauli.py
Outdated
def pauli(ind: int | str | list[int] | list[str], is_sparse: bool = False) -> np.ndarray | sparse.csr_matrix: | ||
def pauli(ind: int | str | list[int] | list[str], is_sparse: bool = False) -> csr_array: |
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.
Why remove np.ndarray
? You get a csr_array
for the output only when we provide is_sparse
is not False
.
I think you could use np.ndarray
as the output type for all allowed input types if we use the .toarray()
option at line 99. WDYT @vprusso ?
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 was thinking back to: #521 (comment) and that's why initially I removed is_sparse
and made some changes to the test cases. But I'll wait for a further response.
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, but issparse
is a method for both sparse array and sparse matrix types. So, the structure of the function shouldn't change that much in this PR. Before, the return type was an array or a sparse matrix. Now, it can be an np.ndarray
for all options of allowed inputs.
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.
LGTM! I'll wait for @vprusso to Ok this before merging.
Yep, all good here as well. Merge when ready! Thank you again for the contribution, @Bchass ! 🚀 |
Description
Closes: #529
I left
is_sparse
in for the moment even though it's not technically being used inpauli.py
due to the test cases relying on True or False. Would it make sense to completely removeis_sparse
and dropTrue
from the test cases? @purva-thakre I believe so, but drafting this for PR for now.Changes
csr_matrix
Checklist
ruff
andpylint
for errors related to code style and formatting.pytest
.Sphinx
build can be checked locally for any failures related to your PRlinkcheck
to check for broken links in the documentationdoctest
to verify the examples in the function docstrings work as expected.