Make findn and findnz behavior match nnz

[andreasnoack]

such that length of returned vectors is nnz.

This is a minimal fix of #23121.

Longer term we should consider the names of these functions. It has previously been proposed that the n and nz part should be something like filled/stored. However, there is also an inconsistency in the find part because some find functions only return a (linear) index (even it doesn't makes sense such as for dictionaries) while others return both indices and values.

[nalimilan]

Longer term we should consider the names of these functions. It has previously been proposed that the n and nz part should be something like filled/stored. However, there is also an inconsistency in the find part because some find functions only return a (linear) index (even it doesn't makes sense such as for dictionaries) while others return both indices and values.

Longer term is pretty short these days. :-)

I agree it would make sense to restrict find* functions to return indices, and that findnz and findmin/findmax are outliers. The latter should be equivalent to indmin/indmax since that's more in line with other find functions.

Maybe findnz could be renamed to stored or filled and return a NamedTuple? nonzeros could then be stored(x).values. nnz could even be length(stored(x).values) if we want, but that's optional.

[andreasnoack]

The latter should be equivalent to indmin/indmax since that's more in line with other find functions

Why not the other way around? Having the value seems useful.

Maybe findnz could be renamed to stored or filled and return a NamedTuple?

I don't understand the reason for this suggestion. Could you explain?

nnz could even be length(stored(x).values) if we want

Probably not. This would be much more expensive than the current nnz that basically just looks up two elements.

[nalimilan]

The latter should be equivalent to indmin/indmax since that's more in line with other find functions

Why not the other way around? Having the value seems useful.

That's useful, but other find functions return only the index, so we'd better find a different name for this operation.

Maybe findnz could be renamed to stored or filled and return a NamedTuple?

I don't understand the reason for this suggestion. Could you explain?

The goal was in the next sentence: "nonzeros could then be stored(x).values. " And renaming findnz would again be more consistent with other find* methods returning only linear indices, not values.

[andreasnoack]

That's useful, but other find functions return only the index, so we'd better find a different name for this operation.

Since it is currently not consistent with findmin, findmax, and findnz returning the values and not just the indices I don't think it would be crazy to go the other way and adjust the other find methods. No big deal though. However, I think the linear index part should be reconsidered since it is from a time when everything was an Array. E.g. linear index for dictionaries doesn't make a lot of sense. I think the current behavior of findmin/max(::Dict) is the right solution, i.e. the key is the index

julia> d = Dict('A' => 1, 'B' => -1)
Dict{Char,Int64} with 2 entries:
  'A' => 1
  'B' => -1

julia> findmax(d)
(1, 'A')

and similar return cartesian indices array types that have IndexCartesian.

[nalimilan]

Since it is currently not consistent with findmin, findmax, and findnz returning the values and not just the indices I don't think it would be crazy to go the other way and adjust the other find methods. No big deal though.

Well, at least we have a consistent proposal to use find* functions in the Search & Find Julep, so I find it simpler to rename other functions.

However, I think the linear index part should be reconsidered since it is from a time when everything was an Array. E.g. linear index for dictionaries doesn't make a lot of sense. I think the current behavior of findmin/max(::Dict) is the right solution, i.e. the key is the index

See JuliaLang/Juleps#27 and #20684. I think we need to discuss this seriously before 0.7.

[andreasnoack]

I'll merge this one since the renaming can happen independently from this change which just makes the behavior more consistent.

[Sacha0]

Have we a tracking issue for the tangential topics? And if so, should we apply a triage label? Best!

[nalimilan]

See #24865 about findmin/findmax.

[nalimilan]

This PR should have updated the SparseVector method too. See #24910 (comment).