Change findfirst/findlast/findnext/findprev to return the same index type as keys() #25577
Conversation
base/array.jl
Outdated
| ``` | ||
| """ | ||
| findfirst(A) = findnext(A, 1) | ||
| findfirst(A) = isempty(A) ? nothing : findnext(A, first(keys(A))) |
There was a problem hiding this comment.
Calling isempty seems to be necessary for full generality, since there is no guaranty that keys returns a range, and first could throw an error for non-ranges. Yet it's quite annoying to have to do that. Actually, this should be done in findnext, but I wonder about the performance impact, given that findnext isn't likely to be inlined (so the check will be performed on every call).
That's related to #25385. Maybe we should require that keys returns something on which first never fails for numeric indices?
Following that idea, I've added a first(::EachStringIndex) = 1 below for consistency with ranges.
There was a problem hiding this comment.
If findfirst is going to be generalized this much, it probably makes sense to rewrite it to iterate over pairs instead of calling findnext.
There was a problem hiding this comment.
Of course, my focus on findnext/findprev made me forget the big picture. I've added a commit to use the most generic approach, similar to find.
| function Base.nextind(a::AbstractArray{<:Any,N}, i::CartesianIndex{N}) where {N} | ||
| _, ni = next(CartesianIndices(axes(a)), i) | ||
| return ni | ||
| end | ||
| function Base.prevind(a::AbstractArray{<:Any,N}, i::CartesianIndex{N}) where {N} | ||
| _, ni = next(Iterators.reverse(CartesianIndices(axes(a))), i) |
There was a problem hiding this comment.
This works but looks a bit convoluted. Is there are simpler way of doing this? @timholy
| @@ -1492,26 +1492,45 @@ cat(n::Integer, x::Integer...) = reshape([x...], (ntuple(x->1, n-1)..., length(x | |||
|
|
|||
| ## find ## | |||
|
|
|||
| _pairs(A::Union{AbstractArray, AbstractDict, AbstractString, Tuple, NamedTuple}) = pairs(A) | |||
| _pairs(iter) = _pairs(IteratorSize(iter), iter) | |||
| _pairs(::Union{HasLength, HasShape}, iter) = zip(1:length(iter), iter) | |||
There was a problem hiding this comment.
When writing this I realized we could return CartesianIndex objects for HasShape iterators. But for consistency with arrays (and for convenience), we should return Int linear indices rather than one-dimensional CartesianIndex objects for 1D HasShape iterators. That's not possible at the moment without adding dimension type parameter, i.e. HasShape{N} as in #25356 (comment). Can easily be changed in a subsequent PR.
…type as keys() This is consistent with find(). We need to check whether the iterator is empty first, since calling first(keys(A)) may throw an error. Also add needed last(::EachStringIndex) and prevind(::AbstractArray, ::CartesianIndex) methods.
Also add tests, and remove @inferred calls which did not actually check whether functions were inferred, but only whether the equality test was. Change the _pairs() fallback to use Iterators.countfrom() so that an error is raised by findlast() rather than assuming the last index is typemax(Int), and use length() for iterators which support it so that findlast() works.
nalimilan
force-pushed
the
nl/findfirstlast
branch
from
bd772d7
to
80e4d44
Compare
…/`argmax` This is the sentence used for `find*` functions, introduced by #25577. Also change "the domain of `f`" to "`domain`" as the domain of `f` can be a superset of the passed `domain`.
…/`argmax` (#46705) This is the sentence used for `find*` functions, introduced by #25577. Also change "the domain of `f`" to "`domain`" as the domain of `f` can be a superset of the passed `domain`. (Spotted at JuliaData/DataFrames.jl#3139.) --------- Co-authored-by: Jameson Nash <vtjnash@gmail.com> Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com>
This is consistent with
find. Also add neededlast(::EachStringIndex)andprevind(::AbstractArray, ::CartesianIndex)methods.Part of #10593.
findnext/findprevare not as generic asfind/findfirst/findlastsince their interface assumes that indices are ordered (comparable with<) and that they are an efficient way of keeping track of the iteration state. This works for arrays, tuples and strings, but not e.g. for dicts (and not even e.g. forOrderedDict, since the ordering of keys isn't reflected by<). Only afindeachfunction returning an iterator would allow that (we can always introduce it later).It would be possible to support passing linear indices to
findnext/findprev, and have them return linear indices in that case. This behavior would probably have to be restricted toAbstractArray. That would limit breakage compared with 0.6 a bit (thoughfindfirstandfindlastwould still break). Should I add it?