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

Renaming findn and findnz? #24910

Closed
andreasnoack opened this issue Dec 4, 2017 · 14 comments · Fixed by #25641
Closed

Renaming findn and findnz? #24910

andreasnoack opened this issue Dec 4, 2017 · 14 comments · Fixed by #25641
Labels
search & find The find* family of functions

Comments

@andreasnoack
Copy link
Member

As discussed in #24724 these behave slightly different from other functions so it might be worth renaming them as part of the general work described in https://github.com/JuliaLang/Juleps/blob/master/Find.md

@ararslan ararslan added the search & find The find* family of functions label Dec 5, 2017
@nalimilan
Copy link
Member

As noted at #24724, findn wouldn't be needed if find returned CartesianIndexes for matrices, as #24774 does. findn(x) could then be find(!iszero, x), with a specialized sparse matrix method for efficiency.

findnz could be renamed to stored or filled, which would have the advantage of moving away from the too restrictive "zero"/"nonzero" terminology. nnz should also be renamed at the same time.

@Sacha0
Copy link
Member

Sacha0 commented Dec 6, 2017

findnz could be renamed to stored or filled, which would have the advantage of moving away from the too restrictive "zero"/"nonzero" terminology. nnz should also be renamed at the same time.

👍 Revising the names of sparse array type fields and associated functions to better reflect present semantics (and possibly enable SparseVector-SparseMatrixCSC unification) is on my list of hypothetical post-1.0 projects, so this direction sounds great on this end :).

@nalimilan
Copy link
Member

@Sacha0 What concrete changes do you think should happen before 1.0 here? Renaming findnz to stored and nnz to something else?

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Dec 15, 2017

How about Sparse.stored and Sparse.nstored?

@Sacha0
Copy link
Member

Sacha0 commented Dec 16, 2017

@Sacha0 What concrete changes do you think should happen before 1.0 here? Renaming findnz to stored and nnz to something else?

The findn(x) -> find(!iszero, x) change sounds lovely and realistic for 1.0. Is that change pending or has it already gone through?

findnz(A) generates a COO (coordinate) representation of A from its nonzeros, independent of what A stores. A name reflecting that behavior seems best? (stored does not quite capture that behavior, and perhaps should be reserved for other uses in a future overhaul of sparse arrays.)

nnz -> numstored is what I had in mind for a future overhaul; I prefer numstored's clarity to nstored :). Best!

@nalimilan
Copy link
Member

nalimilan commented Dec 16, 2017

The findn(x) -> find(!iszero, x) change sounds lovely and realistic for 1.0. Is that change pending or has it already gone through?

It's still pending, blocked by #24774. I think we should merge that PR anyway and take care of performance later.

findnz(A) generates a COO (coordinate) representation of A from its nonzeros, independent of what A stores. A name reflecting that behavior seems best? (stored does not quite capture that behavior, and perhaps should be reserved for other uses in a future overhaul of sparse arrays.)

OK. Do we really need both findn (or it's replacement) and findnz? I'm not very clear about their differences.

nnz -> numstored is what I had in mind for a future overhaul; I prefer numstored's clarity to nstored :).

I prefer nstored, since "num" evokes "numeric" and because we already have e.g. nfield. But that's not a big deal.

To be honest, I wouldn't mind some help for issues which are not blocked by progress on my other PRs. :-)

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Dec 16, 2017

It's still pending, blocked by #24724.

That's merged already though...

@Sacha0
Copy link
Member

Sacha0 commented Dec 16, 2017

OK. Do we really need both findn (or it's replacement) and findnz? I'm not very clear about their differences.

Insofar as I am concerned, no. I can't remember the last time I used findnz; usually a better way to achieve whatever you want to achieve with it exists :).

@nalimilan
Copy link
Member

That's merged already though...

Woops, sorry, I meant #24774.

@nalimilan
Copy link
Member

#25532 deprecates findn. findnz could either be also deprecated, or moved to the new SparseArrays stdlib module and possibly renamed.

@Sacha0
Copy link
Member

Sacha0 commented Jan 16, 2018

Only findnz remains at issue with #25532 merged? Thoughts regarding the best course of action with findnz? Best!

@StefanKarpinski
Copy link
Member

Thoughts regarding the best course of action with findnz?

Kill it with fire. Well, at least rename it 😛 – the name doesn't at all suggest what it does.

@nalimilan
Copy link
Member

nalimilan commented Jan 16, 2018

I've just realized the definition of findnz is totally inconsistent currently. For SparseVector it returns entries which are actually different from 0 (like findn did), but for SparseMatrix it just returns stored entries (zero or not). This confusion is probably due to the poor naming choice.

So I can see two scenarios:

  • findnz is really about stored entries: move it to SparseArrays now, and rename it at some point to change "nz" into "stored" or "filled" (there are several functions with "nz" that in that module). Fix the SparseVector method to keep stored zeros.
  • findnz(x) is the equivalent of find(!iszero, x) except that it also returns the values. So it's really the same naming question as for findmin/findmax (Rename findmin and findmax? #24865). Either deprecate it in favor of find(!iszero, x) just like we just did for findn(!iszero, x), and reintroduce something with a different name if people complain, or find a name for this pattern immediately.

EDIT: actually the inconsistency has been recently introduced by #24724, which forgot to fix SparseVector. So it appears we have chosen the first scenario.

@nalimilan
Copy link
Member

#25641 moves findnz to the SparseArrays module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
search & find The find* family of functions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants