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

nnz and nonzeros change #6761

Closed
ghost opened this issue May 6, 2014 · 9 comments
Closed

nnz and nonzeros change #6761

ghost opened this issue May 6, 2014 · 9 comments
Labels
Milestone

Comments

@ghost
Copy link

ghost commented May 6, 2014

It seems that in the new version nnz was replaced with coutnz and that nonzeros is not working.

Is this by design?

@ivarne
Copy link
Sponsor Member

ivarne commented May 6, 2014

Yes, it's by design. If you search for countnz (watch your spelling!), you find #5424, where the rationale is explained.

The problem is that some operations might store zeros in a sparse matrix and there will be two reasonable behaviours of nnz . Now there are 2 functions with hopefully somewhat more self explaining names. countnz traverses the sparse structure to check all stored elements if they are 0. nfilled is to find out how many elements are actually stored in the matrix (some of which might be zero).

@ViralBShah ViralBShah added the doc label May 6, 2014
@ViralBShah ViralBShah added this to the 0.3 milestone May 6, 2014
@ViralBShah
Copy link
Member

Perhaps we need better documentation of this. I am marking this as a doc issue for 0.3.

@JeffBezanson
Copy link
Sponsor Member

The fact that sparse matrices have been broken to no longer compute nnz in constant time is not a good reason to rename the function. The function has well-defined behavior for any iterable: it counts elements for which x != 0 is true. For sparse, renaming the function after making it slower adds insult to injury. If it can't perform well, it can at least be backwards compatible.

@JeffBezanson
Copy link
Sponsor Member

I should add that while nfilled is perfectly fine to have, it would be very dangerous for users to be lured into using it due to its being O(1). I can't believe the docs recommend calling it. This function does not tell you anything about the array, only a representation detail.

@tkelman
Copy link
Contributor

tkelman commented May 6, 2014

Except for many algorithm implementations that deal with sparse matrices at more than a superficial level, the representation details are the most important part. Matlab's sparse matrices are broken for many purposes and require hacky slow workarounds due to not allowing explicit zeros. This is a separate question from the naming of the functions (or improving the docs to fully clarify the distinction), but I don't see any way to retain constant-time nnz while fixing that deficiency.

@JeffBezanson
Copy link
Sponsor Member

That's fine with me; I will leave the design to the sparse matrix experts. We can keep the way it works now, but should bring back nnz and nonzeros. nfilled is there for experts and we can just clarify in the docs that it does not mean nnz.

@ViralBShah
Copy link
Member

Although I never liked it, I am seriously contemplating having a flag for stored zeros.

@ViralBShah ViralBShah removed the doc label May 7, 2014
@ViralBShah
Copy link
Member

Same as #6769. Closing this one.

@timholy
Copy link
Sponsor Member

timholy commented May 7, 2014

I'm no expert, but I'm seconding @tkelman's point. I have a reasonably large chunk of Matlab code which needs to iteratively zero-out elements of a sparse matrix. I basically ended up developing my own sparse matrix representation (as a struct array, one per row or column) because Matlab's sparse matrices were completely useless for this kind of problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants