From 29831a1e151354b03e50af622dc1c7b4501c8abc Mon Sep 17 00:00:00 2001 From: Milan Bouchet-Valat Date: Sat, 17 Feb 2018 15:58:40 +0100 Subject: [PATCH 1/2] Drop support for non-indexable iterators in find* functions Always call pairs(), which will throw an error for iterables which do not define it. Removing the internal _pairs() function allows supporting custom types which implement pairs(). Also fix/improve a few docstrings. --- base/array.jl | 53 ++++++++++++++++-------------------------------- test/arrayops.jl | 45 ---------------------------------------- 2 files changed, 17 insertions(+), 81 deletions(-) diff --git a/base/array.jl b/base/array.jl index 458dab4ef1356..7d864d8e98880 100644 --- a/base/array.jl +++ b/base/array.jl @@ -1497,17 +1497,10 @@ 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) -# includes HasShape{1} for consistency with keys(::AbstractVector) -_pairs(::Union{HasLength, HasShape{1}}, iter) = zip(1:length(iter), iter) -_pairs(::HasShape, iter) = zip(CartesianIndices(size(iter)), iter) -_pairs(::Union{SizeUnknown, IsInfinite}, iter) = zip(Iterators.countfrom(1), iter) - """ - findnext(A, i::Integer) + findnext(A, i) -Find the next linear index >= `i` of a `true` element of `A`, or `nothing` if not found. +Find the next index after `i` of a `true` element of `A`, or `nothing` if not found. Indices are of the same type as those returned by [`keys(A)`](@ref) and [`pairs(A)`](@ref). @@ -1562,9 +1555,7 @@ Return `nothing` if no such value is found. To search for other kinds of values, pass a predicate as the first argument. Indices or keys are of the same type as those returned by [`keys(A)`](@ref) -and [`pairs(A)`](@ref) for `AbstractArray`, `AbstractDict`, `AbstractString` -`Tuple` and [`NamedTuple`](@ref) objects, and are linear indices starting at `1` -for other iterables. +and [`pairs(A)`](@ref). # Examples ```jldoctest @@ -1592,7 +1583,7 @@ CartesianIndex(2, 1) """ function findfirst(A) warned = false - for (i, a) in _pairs(A) + for (i, a) in pairs(A) if !warned && !(a isa Bool) depwarn("In the future `findfirst` will only work on boolean collections. Use `findfirst(x->x!=0, A)` instead.", :findfirst) warned = true @@ -1610,7 +1601,7 @@ findfirst(A::Union{AbstractArray, AbstractString}) = findnext(A, first(keys(A))) """ findnext(predicate::Function, A, i) -Find the next index >= `i` of an element of `A` for which `predicate` returns `true`, +Find the next index after `i` of an element of `A` for which `predicate` returns `true`, or `nothing` if not found. Indices are of the same type as those returned by [`keys(A)`](@ref) @@ -1659,9 +1650,7 @@ Return the index or key of the first element of `A` for which `predicate` return Return `nothing` if there is no such element. Indices or keys are of the same type as those returned by [`keys(A)`](@ref) -and [`pairs(A)`](@ref) for `AbstractArray`, `AbstractDict`, `AbstractString` -`Tuple` and [`NamedTuple`](@ref) objects, and are linear indices starting at `1` -for other iterables. +and [`pairs(A)`](@ref). # Examples ```jldoctest @@ -1691,7 +1680,7 @@ CartesianIndex(2, 1) ``` """ function findfirst(testf::Function, A) - for (i, a) in _pairs(A) + for (i, a) in pairs(A) testf(a) && return i end return nothing @@ -1704,7 +1693,7 @@ findfirst(testf::Function, A::Union{AbstractArray, AbstractString}) = """ findprev(A, i) -Find the previous index <= `i` of a `true` element of `A`, or `nothing` if not found. +Find the previous index before `i` of a `true` element of `A`, or `nothing` if not found. Indices are of the same type as those returned by [`keys(A)`](@ref) and [`pairs(A)`](@ref). @@ -1755,9 +1744,7 @@ Return the index or key of the last `true` value in `A`. Return `nothing` if there is no `true` value in `A`. Indices or keys are of the same type as those returned by [`keys(A)`](@ref) -and [`pairs(A)`](@ref) for `AbstractArray`, `AbstractDict`, `AbstractString` -`Tuple` and [`NamedTuple`](@ref) objects, and are linear indices starting at `1` -for other iterables. +and [`pairs(A)`](@ref). # Examples ```jldoctest @@ -1787,7 +1774,7 @@ CartesianIndex(2, 1) """ function findlast(A) warned = false - for (i, a) in Iterators.reverse(_pairs(A)) + for (i, a) in Iterators.reverse(pairs(A)) if !warned && !(a isa Bool) depwarn("In the future `findlast` will only work on boolean collections. Use `findlast(x->x!=0, A)` instead.", :findlast) warned = true @@ -1805,7 +1792,7 @@ findlast(A::Union{AbstractArray, AbstractString}) = findprev(A, last(keys(A))) """ findprev(predicate::Function, A, i) -Find the previous index <= `i` of an element of `A` for which `predicate` returns `true`, or +Find the previous index before `i` of an element of `A` for which `predicate` returns `true`, or `nothing` if not found. Indices are of the same type as those returned by [`keys(A)`](@ref) @@ -1851,9 +1838,7 @@ Return the index or key of the last element of `A` for which `predicate` returns Return `nothing` if there is no such element. Indices or keys are of the same type as those returned by [`keys(A)`](@ref) -and [`pairs(A)`](@ref) for `AbstractArray`, `AbstractDict`, `AbstractString` -`Tuple` and [`NamedTuple`](@ref) objects, and are linear indices starting at `1` -for other iterables. +and [`pairs(A)`](@ref). # Examples ```jldoctest @@ -1880,7 +1865,7 @@ CartesianIndex(2, 1) ``` """ function findlast(testf::Function, A) - for (i, a) in Iterators.reverse(_pairs(A)) + for (i, a) in Iterators.reverse(pairs(A)) testf(a) && return i end return nothing @@ -1897,9 +1882,7 @@ Return a vector `I` of the indices or keys of `A` where `f(A[I])` returns `true` If there are no such elements of `A`, return an empty array. Indices or keys are of the same type as those returned by [`keys(A)`](@ref) -and [`pairs(A)`](@ref) for `AbstractArray`, `AbstractDict`, `AbstractString` -`Tuple` and [`NamedTuple`](@ref) objects, and are linear indices starting at `1` -for other iterables. +and [`pairs(A)`](@ref). # Examples ```jldoctest @@ -1943,7 +1926,7 @@ julia> findall(x -> x >= 0, d) ``` """ -findall(testf::Function, A) = collect(first(p) for p in _pairs(A) if testf(last(p))) +findall(testf::Function, A) = collect(first(p) for p in pairs(A) if testf(last(p))) """ findall(A) @@ -1953,9 +1936,7 @@ If there are no such elements of `A`, return an empty array. To search for other kinds of values, pass a predicate as the first argument. Indices or keys are of the same type as those returned by [`keys(A)`](@ref) -and [`pairs(A)`](@ref) for `AbstractArray`, `AbstractDict`, `AbstractString` -`Tuple` and [`NamedTuple`](@ref) objects, and are linear indices starting at `1` -for other iterables. +and [`pairs(A)`](@ref). # Examples ```jldoctest @@ -1989,7 +1970,7 @@ function findall(A) if !(eltype(A) === Bool) && !all(x -> x isa Bool, A) depwarn("In the future `findall(A)` will only work on boolean collections. Use `findall(x->x!=0, A)` instead.", :find) end - collect(first(p) for p in _pairs(A) if last(p) != 0) + collect(first(p) for p in pairs(A) if last(p) != 0) end # Allocating result upfront is faster (possible only when collection can be iterated twice) function findall(A::AbstractArray{Bool}) diff --git a/test/arrayops.jl b/test/arrayops.jl index 9b75d2ffad4de..75660e3ff03e9 100644 --- a/test/arrayops.jl +++ b/test/arrayops.jl @@ -475,51 +475,6 @@ end @test findnext(isodd, A, CartesianIndex(1, 2)) === nothing @test findprev(iseven, A, CartesianIndex(2, 1)) === nothing end -@testset "find with general iterables" begin - s = "julia" - @test findall(c -> c == 'l', s) == [3] - @test findfirst(c -> c == 'l', s) == 3 - @test findlast(c -> c == 'l', s) == 3 - @test findnext(c -> c == 'l', s, 1) == 3 - @test findprev(c -> c == 'l', s, 5) == 3 - @test findnext(c -> c == 'l', s, 4) === nothing - @test findprev(c -> c == 'l', s, 2) === nothing - - g = Base.Unicode.graphemes("日本語") - @test findall(!isempty, g) == 1:3 - @test isempty(findall(isascii, g)) - @test findfirst(!isempty, g) == 1 - @test findfirst(isascii, g) === nothing - # Check that the last index isn't assumed to be typemax(Int) - @test_throws MethodError findlast(!iszero, g) - - g2 = (i % 2 for i in 1:10) - @test findall(!iszero, g2) == 1:2:9 - @test findfirst(!iszero, g2) == 1 - @test findlast(!iszero, g2) == 9 - @test findfirst(equalto(2), g2) === nothing - @test findlast(equalto(2), g2) === nothing - - g3 = (i % 2 for i in 1:10, j in 1:2) - @test findall(!iszero, g3) == findall(!iszero, collect(g3)) - @test findfirst(!iszero, g3) == CartesianIndex(1, 1) - @test findlast(!iszero, g3) == CartesianIndex(9, 2) - @test findfirst(equalto(2), g3) === nothing - @test findlast(equalto(2), g3) === nothing - - g4 = (x for x in [true, false, true, false]) - @test findall(g4) == [1, 3] - @test findfirst(g4) == 1 - @test findlast(g4) == 3 - - g5 = (x for x in [true false; true false]) - @test findall(g5) == findall(collect(g5)) - @test findfirst(g5) == CartesianIndex(1, 1) - @test findlast(g5) == CartesianIndex(2, 1) - - @test findfirst(x for x in Bool[]) === nothing - @test findlast(x for x in Bool[]) === nothing -end @testset "findmin findmax argmin argmax" begin @test argmax([10,12,9,11]) == 2 From 31aa07d13d17e1ea002f090d11ae3e30e61dc2da Mon Sep 17 00:00:00 2001 From: Milan Bouchet-Valat Date: Sun, 18 Feb 2018 12:37:03 +0100 Subject: [PATCH 2/2] Fix "after/before or including i" --- base/array.jl | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/base/array.jl b/base/array.jl index 7d864d8e98880..d847cbe19ac08 100644 --- a/base/array.jl +++ b/base/array.jl @@ -1500,7 +1500,8 @@ cat(n::Integer, x::Integer...) = reshape([x...], (ntuple(x->1, n-1)..., length(x """ findnext(A, i) -Find the next index after `i` of a `true` element of `A`, or `nothing` if not found. +Find the next index after or including `i` of a `true` element of `A`, +or `nothing` if not found. Indices are of the same type as those returned by [`keys(A)`](@ref) and [`pairs(A)`](@ref). @@ -1601,8 +1602,8 @@ findfirst(A::Union{AbstractArray, AbstractString}) = findnext(A, first(keys(A))) """ findnext(predicate::Function, A, i) -Find the next index after `i` of an element of `A` for which `predicate` returns `true`, -or `nothing` if not found. +Find the next index after or including `i` of an element of `A` +for which `predicate` returns `true`, or `nothing` if not found. Indices are of the same type as those returned by [`keys(A)`](@ref) and [`pairs(A)`](@ref). @@ -1693,7 +1694,8 @@ findfirst(testf::Function, A::Union{AbstractArray, AbstractString}) = """ findprev(A, i) -Find the previous index before `i` of a `true` element of `A`, or `nothing` if not found. +Find the previous index before or including `i` of a `true` element of `A`, +or `nothing` if not found. Indices are of the same type as those returned by [`keys(A)`](@ref) and [`pairs(A)`](@ref). @@ -1792,8 +1794,8 @@ findlast(A::Union{AbstractArray, AbstractString}) = findprev(A, last(keys(A))) """ findprev(predicate::Function, A, i) -Find the previous index before `i` of an element of `A` for which `predicate` returns `true`, or -`nothing` if not found. +Find the previous index before or including `i` of an element of `A` +for which `predicate` returns `true`, or `nothing` if not found. Indices are of the same type as those returned by [`keys(A)`](@ref) and [`pairs(A)`](@ref).