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.

Sign up for GitHub

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

make findmin/findmax behavior match min/max (fix #23209) #23227

Merged
merged 15 commits into from
Aug 31, 2017
Merged

make findmin/findmax behavior match min/max (fix #23209) #23227

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
b4113e7
findmin findmax in array.jl and reducedim.jl ( fix #23209)
KlausC Aug 11, 2017
172c9ef
fixed issues with Inf, BigInt, and String
KlausC Aug 12, 2017
915d719
fixed reducedim versions of minimum/maximum
KlausC Aug 13, 2017
519184b
removed definition of zero2
KlausC Aug 16, 2017
faf4927
cover sparse matrices #23209
KlausC Aug 16, 2017
2ad37fe
gitspaces...
KlausC Aug 16, 2017
829f0c5
off-by-one bound-check bug in _findz fixed
KlausC Aug 17, 2017
57329bf
Merge remote-tracking branch 'upstream/master' into fix#23209
KlausC Aug 17, 2017
37d8bf3
avoid breaking of test "distributed"
KlausC Aug 17, 2017
6c64a20
offsetarray compatibility - consistend empty case
KlausC Aug 18, 2017
d6cd550
offsetarray compatibility - consistent empty case
KlausC Aug 18, 2017
78bbd14
Merge branch 'fix#23209' of https://github.com/KlausC/julia into fix#…
KlausC Aug 18, 2017
6532ba8
use isequal in tests
KlausC Aug 22, 2017
be0f449
Remove unrelated distributed change
mbauman Aug 24, 2017
a52c2aa
Add NEWS.md
mbauman Aug 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 16 additions & 20 deletions base/array.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,6 @@ function fill!(a::Array{T}, x) where T<:Union{Integer,AbstractFloat}
return a
end


"""
fill(x, dims)

Expand Down Expand Up @@ -777,7 +776,6 @@ function _prepend!(a, ::IteratorSize, iter)
a
end


"""
resize!(a::Vector, n::Integer) -> Vector

Expand Down Expand Up @@ -1181,7 +1179,6 @@ function reverse!(v::AbstractVector, s=first(linearindices(v)), n=last(linearind
return v
end


# concatenations of homogeneous combinations of vectors, horizontal and vertical

vcat() = Array{Any,1}(0)
Expand Down Expand Up @@ -1226,7 +1223,6 @@ end

cat(n::Integer, x::Integer...) = reshape([x...], (ntuple(x->1, n-1)..., length(x)))


## find ##

"""
Expand Down Expand Up @@ -1696,8 +1692,9 @@ end
findmax(itr) -> (x, index)

Returns the maximum element of the collection `itr` and its index. If there are multiple
maximal elements, then the first one will be returned. `NaN` values are ignored, unless
all elements are `NaN`. Other than the treatment of `NaN`, the result is in line with `max`.
maximal elements, then the first one will be returned.
If any data element is `NaN`, this element is returned.
The result is in line with `max`.

The collection must not be empty.

Expand All @@ -1710,7 +1707,7 @@ julia> findmax([1,7,7,6])
(7, 2)

julia> findmax([1,7,7,NaN])
(7.0, 2)
(NaN, 4)
```
"""
function findmax(a)
Expand All @@ -1721,10 +1718,10 @@ function findmax(a)
mi = i = 1
m, s = next(a, s)
while !done(a, s)
m != m && break
ai, s = next(a, s)
i += 1
ai != ai && continue # assume x != x => x is a NaN
if m != m || isless(m, ai)
if ai != ai || isless(m, ai)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the ai != ai case this can short-circuit, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if ai != ai I expect isless is not called. That shall cover the NaN- case. This can occur at most once, since the next loop circle is broken by the m != m condition few lines before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, I didn't notice the m != m && break check previously.

m = ai
mi = i
end
Expand All @@ -1736,8 +1733,9 @@ end
findmin(itr) -> (x, index)

Returns the minimum element of the collection `itr` and its index. If there are multiple
minimal elements, then the first one will be returned. `NaN` values are ignored, unless
all elements are `NaN`. Other than the treatment of `NaN`, the result is in line with `min`.
minimal elements, then the first one will be returned.
If any data element is `NaN`, this element is returned.
The result is in line with `min`.

The collection must not be empty.

Expand All @@ -1750,7 +1748,7 @@ julia> findmin([7,1,1,6])
(1, 2)

julia> findmin([7,1,1,NaN])
(1.0, 2)
(NaN, 4)
```
"""
function findmin(a)
Expand All @@ -1761,10 +1759,10 @@ function findmin(a)
mi = i = 1
m, s = next(a, s)
while !done(a, s)
m != m && break
ai, s = next(a, s)
i += 1
ai != ai && continue
if m != m || isless(ai, m)
if ai != ai || isless(ai, m)
m = ai
mi = i
end
Expand All @@ -1776,8 +1774,7 @@ end
indmax(itr) -> Integer

Returns the index of the maximum element in a collection. If there are multiple maximal
elements, then the first one will be returned. `NaN` values are ignored, unless all
elements are `NaN`.
elements, then the first one will be returned.

The collection must not be empty.

Expand All @@ -1790,7 +1787,7 @@ julia> indmax([1,7,7,6])
2

julia> indmax([1,7,7,NaN])
2
4
```
"""
indmax(a) = findmax(a)[2]
Expand All @@ -1799,8 +1796,7 @@ indmax(a) = findmax(a)[2]
indmin(itr) -> Integer

Returns the index of the minimum element in a collection. If there are multiple minimal
elements, then the first one will be returned. `NaN` values are ignored, unless all
elements are `NaN`.
elements, then the first one will be returned.

The collection must not be empty.

Expand All @@ -1813,7 +1809,7 @@ julia> indmin([7,1,1,6])
2

julia> indmin([7,1,1,NaN])
2
4
```
"""
indmin(a) = findmin(a)[2]
Expand Down
92 changes: 60 additions & 32 deletions base/reducedim.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,24 +57,33 @@ function reduced_indices0(inds::Indices{N}, region) where N
tuple(rinds...)::typeof(inds)
end


###### Generic reduction functions #####

## extensions to the standard functions for BigInt, String and others

zero2(t::Type) = zero(t)
zero2(::Type{T}) where T<:Number = zero(T)
zero2(::Type{T}) where T<:AbstractString = ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requiring this is a bit of a red flag, is there any way to avoid needing this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way is not to rely on zero behavior at all, but to use the first slice as the start value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me improve this!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The definition of zero2 will be removed in the next commit, it is not required any longer.
I guess the red flag is risen, because a bug fix shall not introduce new Base features?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as this isn't exported, it's not a feature addition. It just seems fishy to need a function like zero but for which "" is the answer for strings. (Nevermind that "" is a reasonable 1 value for strings; there is no zero value unless you embed strings into regular patterns, in which case the empty pattern is the zero value – but it's not a string.)


## initialization

for (Op, initfun) in ((:(typeof(+)), :zero), (:(typeof(*)), :one), (:(typeof(scalarmax)), :typemin), (:(typeof(scalarmin)), :typemax), (:(typeof(max)), :typemin), (:(typeof(min)), :typemax))
@eval initarray!(a::AbstractArray{T}, ::$(Op), init::Bool) where {T} = (init && fill!(a, $(initfun)(T)); a)
for (Op, initfun) in ((:(typeof(+)), :zero), (:(typeof(*)), :one))
@eval initarray!(a::AbstractArray{T}, ::$(Op), init::Bool, src::AbstractArray) where {T} = (init && fill!(a, $(initfun)(T)); a)
end

for Op in (:(typeof(scalarmax)), :(typeof(scalarmin)), :(typeof(max)), :(typeof(min)))
@eval initarray!(a::AbstractArray{T}, ::$(Op), init::Bool, src::AbstractArray) where {T} = (init && copyfirst!(a, src); a)
end

for (Op, initval) in ((:(typeof(&)), true), (:(typeof(|)), false))
@eval initarray!(a::AbstractArray, ::$(Op), init::Bool) = (init && fill!(a, $initval); a)
@eval initarray!(a::AbstractArray, ::$(Op), init::Bool, src::AbstractArray) = (init && fill!(a, $initval); a)
end

reducedim_initarray(A::AbstractArray, region, v0, ::Type{R}) where {R} = fill!(similar(A,R,reduced_indices(A,region)), v0)
reducedim_initarray(A::AbstractArray, region, v0::T) where {T} = reducedim_initarray(A, region, v0, T)

reducedim_initarray0(A::AbstractArray, region, v0, ::Type{R}) where {R} = fill!(similar(A,R,reduced_indices0(A,region)), v0)
reducedim_initarray0(A::AbstractArray, region, v0::T) where {T} = reducedim_initarray0(A, region, v0, T)
reducedim_initarray0(A::AbstractArray, region, f) = f.(getindex(A, reduced_indices0(A, region)...))
reducedim_initarray0(A::AbstractArray, region, ::typeof(identity)) = getindex(A, reduced_indices0(A, region)...)

# TODO: better way to handle reducedim initialization
#
Expand All @@ -91,7 +100,7 @@ function reducedim_init(f, op::typeof(*), A::AbstractArray, region)
end
function _reducedim_init(f, op, fv, fop, A, region)
T = promote_union(eltype(A))
if method_exists(zero, Tuple{Type{T}})
if applicable(zero, T)
x = f(zero(T))
z = op(fv(x), fv(x))
Tr = typeof(z) == typeof(x) && !isbits(T) ? T : typeof(z)
Expand All @@ -106,8 +115,8 @@ reducedim_init(f, op::typeof(max), A::AbstractArray, region) = reducedim_init(f,
reducedim_init(f, op::typeof(min), A::AbstractArray, region) = reducedim_init(f, scalarmin, A, region)
reducedim_init(f::Union{typeof(abs),typeof(abs2)}, op::typeof(max), A::AbstractArray, region) = reducedim_init(f, scalarmax, A, region)

reducedim_init(f, op::typeof(scalarmax), A::AbstractArray{T}, region) where {T} = reducedim_initarray0(A, region, typemin(f(zero(T))))
reducedim_init(f, op::typeof(scalarmin), A::AbstractArray{T}, region) where {T} = reducedim_initarray0(A, region, typemax(f(zero(T))))
reducedim_init(f, op::typeof(scalarmax), A::AbstractArray{T}, region) where {T} = reducedim_initarray0(A, region, f)
reducedim_init(f, op::typeof(scalarmin), A::AbstractArray{T}, region) where {T} = reducedim_initarray0(A, region, f)
reducedim_init(f::Union{typeof(abs),typeof(abs2)}, op::typeof(scalarmax), A::AbstractArray{T}, region) where {T} =
reducedim_initarray(A, region, zero(f(zero(T))))

Expand All @@ -132,7 +141,6 @@ end
reducedim_init(f::Union{typeof(identity),typeof(abs),typeof(abs2)}, op::typeof(+), A::AbstractArray{Bool}, region) =
reducedim_initarray(A, region, 0)


## generic (map)reduction

has_fast_linear_indexing(a::AbstractArray) = false
Expand Down Expand Up @@ -169,6 +177,20 @@ function check_reducedims(R, A)
return lsiz
end

"""
Extract first entry of slices of array A into existing array R.
"""
function copyfirst!(R::AbstractArray, A::AbstractArray)
lsiz = check_reducedims(R, A)
iA = collect(map(length, indices(A)))
iR = map!(length, ones(ndims(A)), collect(indices(R)))
t = []
for i in 1:length(iR)
push!(t, iA[i] == iR[i] ? Colon() : 1)
end
copy!(R, view(A, t...))
end

function _mapreducedim!(f, op, R::AbstractArray, A::AbstractArray)
lsiz = check_reducedims(R,A)
isempty(A) && return R
Expand Down Expand Up @@ -211,7 +233,7 @@ mapreducedim!(f, op, R::AbstractArray, A::AbstractArray) =
(_mapreducedim!(f, op, R, A); R)

reducedim!(op, R::AbstractArray{RT}, A::AbstractArray) where {RT} =
mapreducedim!(identity, op, R, A, zero(RT))
mapreducedim!(identity, op, R, A, zero2(RT))

"""
mapreducedim(f, op, A, region[, v0])
Expand Down Expand Up @@ -277,7 +299,6 @@ julia> reducedim(max, a, 1)
reducedim(op, A::AbstractArray, region, v0) = mapreducedim(identity, op, A, region, v0)
reducedim(op, A::AbstractArray, region) = mapreducedim(identity, op, A, region)


##### Specific reduction functions #####
"""
sum(A, dims)
Expand Down Expand Up @@ -577,7 +598,7 @@ for (fname, op) in [(:sum, :+), (:prod, :*),
fname! = Symbol(fname, '!')
@eval begin
$(fname!)(f::Function, r::AbstractArray, A::AbstractArray; init::Bool=true) =
mapreducedim!(f, $(op), initarray!(r, $(op), init), A)
mapreducedim!(f, $(op), initarray!(r, $(op), init, A), A)
$(fname!)(r::AbstractArray, A::AbstractArray; init::Bool=true) = $(fname!)(identity, r, A; init=init)

$(fname)(f::Function, A::AbstractArray, region) =
Expand All @@ -586,9 +607,9 @@ for (fname, op) in [(:sum, :+), (:prod, :*),
end
end


##### findmin & findmax #####

# The initial values of Rval are not used if the correponding indices in Rind are 0.
#
function findminmax!(f, Rval, Rind, A::AbstractArray{T,N}) where {T,N}
(isempty(Rval) || isempty(A)) && return Rval, Rind
lsiz = check_reducedims(Rval, A)
Expand All @@ -609,7 +630,7 @@ function findminmax!(f, Rval, Rind, A::AbstractArray{T,N}) where {T,N}
for i in indices(A,1)
k += 1
tmpAv = A[i,IA]
if f(tmpAv, tmpRv)
if tmpRi == 0 || (tmpRv == tmpRv && (tmpAv != tmpAv || f(tmpAv, tmpRv)))
tmpRv = tmpAv
tmpRi = k
end
Expand All @@ -623,7 +644,9 @@ function findminmax!(f, Rval, Rind, A::AbstractArray{T,N}) where {T,N}
for i in indices(A, 1)
k += 1
tmpAv = A[i,IA]
if f(tmpAv, Rval[i,IR])
tmpRv = Rval[i,IR]
tmpRi = Rind[i,IR]
if tmpRi == 0 || (tmpRv == tmpRv && (tmpAv != tmpAv || f(tmpAv, tmpRv)))
Rval[i,IR] = tmpAv
Rind[i,IR] = k
end
Expand All @@ -633,82 +656,87 @@ function findminmax!(f, Rval, Rind, A::AbstractArray{T,N}) where {T,N}
Rval, Rind
end


"""
findmin!(rval, rind, A, [init=true]) -> (minval, index)

Find the minimum of `A` and the corresponding linear index along singleton
dimensions of `rval` and `rind`, and store the results in `rval` and `rind`.
`NaN` is treated as less than all other values.
"""
function findmin!(rval::AbstractArray, rind::AbstractArray, A::AbstractArray;
init::Bool=true)
findminmax!(<, initarray!(rval, scalarmin, init), rind, A)
findminmax!(isless, init && !isempty(A) ? fill!(rval, first(A)) : rval, fill!(rind,0), A)
end

"""
findmin(A, region) -> (minval, index)

For an array input, returns the value and index of the minimum over the given region.
`NaN` is treated as less than all other values.

# Examples
```jldoctest
julia> A = [1 2; 3 4]
julia> A = [1.0 2; 3 4]
2×2 Array{Int64,2}:
1 2
3 4
1.0 2.0
3.0 4.0

julia> findmin(A, 1)
([1 2], [1 3])
([1.0 2.0], [1 3])

julia> findmin(A, 2)
([1; 3], [1; 2])
([1.0; 3.0], [1; 2])
```
"""
function findmin(A::AbstractArray{T}, region) where T
if isempty(A)
return (similar(A, reduced_indices0(A, region)),
similar(dims->zeros(Int, dims), reduced_indices0(A, region)))
end
return findminmax!(<, reducedim_initarray0(A, region, typemax(T)),
return findminmax!(isless, fill!(similar(A, reduced_indices0(A, region)), first(A)),
similar(dims->zeros(Int, dims), reduced_indices0(A, region)), A)
end

isgt(a, b) = isless(b,a)

"""
findmax!(rval, rind, A, [init=true]) -> (maxval, index)

Find the maximum of `A` and the corresponding linear index along singleton
dimensions of `rval` and `rind`, and store the results in `rval` and `rind`.
`NaN` is treated as greater than all other values.
"""
function findmax!(rval::AbstractArray, rind::AbstractArray, A::AbstractArray;
init::Bool=true)
findminmax!(>, initarray!(rval, scalarmax, init), rind, A)
findminmax!(isgt, init && !isempty(A) ? fill!(rval, first(A)) : rval, fill!(rind,0), A)
end

"""
findmax(A, region) -> (maxval, index)

For an array input, returns the value and index of the maximum over the given region.
`NaN` is treated as greater than all other values.

# Examples
```jldoctest
julia> A = [1 2; 3 4]
julia> A = [1.0 2; 3 4]
2×2 Array{Int64,2}:
1 2
3 4
1.0 2.0
3.0 4.0

julia> findmax(A,1)
([3 4], [2 4])
([3.0 4.0], [2 4])

julia> findmax(A,2)
([2; 4], [3; 4])
([2.0; 4.0], [3; 4])
```
"""
function findmax(A::AbstractArray{T}, region) where T
if isempty(A)
return (similar(A, reduced_indices0(A,region)),
similar(dims->zeros(Int, dims), reduced_indices0(A,region)))
end
return findminmax!(>, reducedim_initarray0(A, region, typemin(T)),
return findminmax!(isgt, fill!(similar(A, reduced_indices0(A, region)), first(A)),
similar(dims->zeros(Int, dims), reduced_indices0(A, region)), A)
end

Expand Down
Loading