Skip to content

Conversation

@ShreyasKhandekar
Copy link
Contributor

@ShreyasKhandekar ShreyasKhandekar commented Jul 27, 2024

Adds printing support for Sparse Matrices as follows:

>>> a = ak.random_sparse_matrix(100, 0.05, "CSC")
>>> print(a)
  (4, 1)        0
  (8, 1)        0
  (22, 1)       0
  :     :
  (63, 100)     0
  (97, 100)     0
  (98, 100)     0

>>> a = ak.random_sparse_matrix(100, 0.05, "CSR")
>>> print(a)
  (1, 5)        0
  (1, 51)       0
  (1, 53)       0
  :     :
  (100, 29)     0
  (100, 52)     0
  (100, 95)     0

This matches how SciPy prints their sparse matrices.

Also added a new nnz member to the sparrayclass to easily query the number of non zeroes in the sparse matrix.

Due to the vertical format of printing here, I've kept the threshold to 20 NNZ instead of 100 which is the standard for pdarrays.

Details in PR

Signed-off-by: Shreyas Khandekar <60454060+ShreyasKhandekar@users.noreply.github.com>
mypy really wants the sparray type to be indexable, but that is not in
the scope of this PR. I can make it the case in a future PR, but for now
I would like to silence the error and move on.
Therefore I added a __get_item__ method to the sparray class which just
raises a NotImplementedError.

Signed-off-by: Shreyas Khandekar <60454060+ShreyasKhandekar@users.noreply.github.com>
Matching what SciPy does mostly.

Signed-off-by: Shreyas Khandekar <60454060+ShreyasKhandekar@users.noreply.github.com>
As well as mypy built succesfully here

Signed-off-by: Shreyas Khandekar <60454060+ShreyasKhandekar@users.noreply.github.com>
@ShreyasKhandekar ShreyasKhandekar marked this pull request as ready for review July 29, 2024 18:25
Copy link
Contributor

@bmcdonald3 bmcdonald3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks Shreyas.

@bmcdonald3 bmcdonald3 added this pull request to the merge queue Jul 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 29, 2024
@ShreyasKhandekar ShreyasKhandekar changed the title Sparse matrix printing and testing Sparse matrix printing Jul 29, 2024
@ShreyasKhandekar
Copy link
Contributor Author

Looks like the merge failed due to some sort of error getting the hdf5 package for testing.

The error I see is as follows:

Setting up libhdf5-dev (1.10.6+repack-4+deb11u1) ...
update-alternatives: using /usr/lib/x86_64-linux-gnu/pkgconfig/hdf5-serial.pc to provide /usr/lib/x86_64-linux-gnu/pkgconfig/hdf5.pc (hdf5.pc) in auto mode
Processing triggers for libc-bin (2.31-13+deb11u10) ...
Installing iconv
rm -rf /__w/arkouda/arkouda//dep/build/libiconv-1.17 /__w/arkouda/arkouda//dep/libiconv-install
mkdir -p /__w/arkouda/arkouda//dep /__w/arkouda/arkouda//dep/build
cd /__w/arkouda/arkouda//dep/build && curl -sL https://ftp.gnu.org/pub/gnu/libiconv/libiconv-1.17.tar.gz | tar xz

gzip: stdin: unexpected end of file
tar: Child returned status 1
tar: Error is not recoverable: exiting now
make: *** [Makefile:169: install-iconv] Error 2

This looks unrelated to my PR and is probably some sort of sporadic network error. I would just try adding this to the merge queue again and hope it passes this time.

@bmcdonald3 bmcdonald3 added this pull request to the merge queue Jul 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 29, 2024
@ShreyasKhandekar
Copy link
Contributor Author

It looks like there was an issue on the gnu side of things, a broken link for libiconv maybe?
#3594 (review) might be referring to the same thing we're seeing in this PR, so if it is fixed then we can just try merging again.

Copy link
Member

@stress-tess stress-tess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! And yah like @ShreyasKhandekar mentioned ftp.gnu.org was down for a bit, see #3595

But it's up now so I'll readd this to the merge queue

@stress-tess stress-tess added this pull request to the merge queue Aug 1, 2024
Merged via the queue into Bears-R-Us:master with commit 8188bcb Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants