Skip to content
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

SparseMatrix Interface #8720

Merged
merged 2 commits into from
Oct 26, 2014
Merged

SparseMatrix Interface #8720

merged 2 commits into from
Oct 26, 2014

Conversation

quinnj
Copy link
Member

@quinnj quinnj commented Oct 18, 2014

Add rowvals(A) and nzrange(A, col) to make the sparse matrix interface a little friendlier.

cc @ViralBShah, @tanmaykm


.. function:: rowvals(A)

Return a vector of the row indices of a sparse matrix ``A``. The returned vector points directly to the internal row indices of ``A``, and any modifications to the returned vector will mutate ``A`` as well.
Copy link
Member

Choose a reason for hiding this comment

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

This description would be pretty confusing to someone not familiar with CSC format. Maybe a bit more context?

@tkelman
Copy link
Contributor

tkelman commented Oct 19, 2014

No reason not to have convenience functions like these. What I'd really like to see is dense and sparse matrices supporting similar "iterate by columns through nonzero elements" (or by rows for transpose types?) API's, so various operators and reductions can extend efficiently to sparse while reusing generic implementations.

@ViralBShah
Copy link
Member

The iterator would be nice but getting the row index and passing it was slow when I first tried this, and I don't think it would have changed.

@johnmyleswhite
Copy link
Member

I was thinking about this recently and though that adding an enumerate method to both dense and sparse arrays would be great.

@ViralBShah ViralBShah added the sparse Sparse arrays label Oct 25, 2014
@ViralBShah
Copy link
Member

How about calling it rowval so that it matches the internal structure, and also having nzval(A)?

@quinnj
Copy link
Member Author

quinnj commented Oct 26, 2014

Updated with suggestions.

@mlubin
Copy link
Member

mlubin commented Oct 26, 2014

Given the history of heated debates on naming (#6769), what's the motivation for renaming nonzeros to nzval? Seems like unnecessary churn, and the whole point of these accessors is that they don't need to match the names of the internal fields.

@ViralBShah
Copy link
Member

Oops, I forgot about nonzeros. I was just mechanically thinking about accessors for the various SparseMatrixCSC fields. We don't need nzval then. Sorry for the noise.

@quinnj quinnj force-pushed the jq/friendlysparse branch from ee4ceab to 3739991 Compare October 26, 2014 04:00
@quinnj
Copy link
Member Author

quinnj commented Oct 26, 2014

Ok, reverted the nzval(A) chnage and kept the documentation updates as suggested by @mlubin

@mlubin
Copy link
Member

mlubin commented Oct 26, 2014

thanks, LGTM

ViralBShah added a commit that referenced this pull request Oct 26, 2014
@ViralBShah ViralBShah merged commit 528a137 into master Oct 26, 2014
@quinnj quinnj deleted the jq/friendlysparse branch February 25, 2015 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sparse Sparse arrays
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants