-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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
MAINT: sparse: reformat str and repr for sparse arrays, correct 1D coords, improve dtype looks #20649
Conversation
When changing the str/repr format, I didn't change the refguide doctests to match. The CI reports 47 doctests that need to be updated. I've updated/corrected them. I also changed the term "type" to "dtype" because that is what we are showing... not idx_type or the object's type. |
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.
Overall this looks really nice, thanks!
scipy/sparse/_lil.py
Outdated
if self.nnz == 0: | ||
return val | ||
|
||
val += '\n Coords\tValue\n' |
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.
The base class uses Coords and Values, so we should use Values here too.
Though I wonder: since this doesn't support maxprint, should we even have this override?
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.
Good point -- This str
override could print something very long. And the base class will handle it just fine.
I've removed that method from _lilbase
here.
I've also rebased to include the csr-1d stuff in case that changes anything.
The CI failure seems not to be related to this PR. I believe this |
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'll wait a bit to see if anyone else has comments, otherwise this is good to merge.
Let me restart the failing CI job for |
Good thanks! Rerunning the test seems to have let it pass. :) |
return '\n'.join([(' {}\t{}'.format(*t)) for t in triples]) | ||
def tostr(coords, data): | ||
pairs = zip(zip(*coords), data) | ||
return '\n'.join(f' {idx}\t{val}' for idx, val in pairs) |
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.
Note: in Numpy 2.0+, this will format the indices with np.int32()
around each one. For example:
(np.int32(3), np.int32(1)) 0.2435
The "add unittests" commit adds tests that expose the change in str for numpy 2.0. (somehow doctests are not being tested using numpy 2.0 yet) The "fix numpy 2 printing" commit converts values to python numeric before printing (using If these tests pass I think we're good to start a re-review by @perimosocordiae due to the numpy changes. |
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.
CI errors are unrelated to this change.
Fixes #20378
Fixes #20377
Currently,
str(A)
for sparse A with no stored elements returns the empty string.In
repr(A)
, for 1D arrays, the shape does not appear nicely, e.g. "958 sparse array of type ...". 2D is "4x3 sparse array of type".This PR puts in repr:
format
anddtype
in first line,nnz
andshape
in 2nd line.For
str
, the text from repr is the start, followed by coord/value column titles followed by coord/value info as before, but now it works for 1D. Also, if no stored values exist, therepr
text is returned.