Skip to content

Commit

Permalink
Fix findmax and findmin with iterables
Browse files Browse the repository at this point in the history
The state is not guaranteed to be equivalent to the index
of the element.

(cherry picked from commit bbaaf54)
ref #14086
  • Loading branch information
nalimilan authored and tkelman committed Nov 30, 2015
1 parent 06c9fe3 commit 13a68a0
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 23 deletions.
3 changes: 0 additions & 3 deletions base/abstractarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -408,9 +408,6 @@ start(A::AbstractArray) = (@_inline_meta(); itr = eachindex(A); (itr, start(itr)
next(A::AbstractArray,i) = (@_inline_meta(); (idx, s) = next(i[1], i[2]); (A[idx], (i[1], s)))
done(A::AbstractArray,i) = done(i[1], i[2])

iterstate(i) = i
iterstate(i::Tuple{UnitRange{Int},Int}) = i[2]

# eachindex iterates over all indices. LinearSlow definitions are later.
eachindex(A::AbstractArray) = (@_inline_meta(); eachindex(linearindexing(A), A))

Expand Down
32 changes: 16 additions & 16 deletions base/array.jl
Original file line number Diff line number Diff line change
Expand Up @@ -830,36 +830,36 @@ function findmax(a)
if isempty(a)
throw(ArgumentError("collection must be non-empty"))
end
i = start(a)
mi = i
m, i = next(a, i)
while !done(a, i)
iold = i
ai, i = next(a, i)
s = start(a)
mi = i = 1
m, s = next(a, s)
while !done(a, s)
ai, s = next(a, s)
i += 1
if ai > m || m!=m
m = ai
mi = iold
mi = i
end
end
return (m, iterstate(mi))
return (m, mi)
end

function findmin(a)
if isempty(a)
throw(ArgumentError("collection must be non-empty"))
end
i = start(a)
mi = i
m, i = next(a, i)
while !done(a, i)
iold = i
ai, i = next(a, i)
s = start(a)
mi = i = 1
m, s = next(a, s)
while !done(a, s)
ai, s = next(a, s)
i += 1
if ai < m || m!=m
m = ai
mi = iold
mi = i
end
end
return (m, iterstate(mi))
return (m, mi)
end

indmax(a) = findmax(a)[2]
Expand Down
4 changes: 1 addition & 3 deletions base/multidimensional.jl
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
### Multidimensional iterators
module IteratorsMD

import Base: eltype, length, start, done, next, last, getindex, setindex!, linearindexing, min, max, eachindex, ndims, iterstate
import Base: eltype, length, start, done, next, last, getindex, setindex!, linearindexing, min, max, eachindex, ndims
importall ..Base.Operators
import Base: simd_outer_range, simd_inner_length, simd_index, @generated
import Base: @nref, @ncall, @nif, @nexprs, LinearFast, LinearSlow, to_index
Expand Down Expand Up @@ -59,8 +59,6 @@ immutable CartesianRange{I<:CartesianIndex}
stop::I
end

iterstate{CR<:CartesianRange,CI<:CartesianIndex}(i::Tuple{CR,CI}) = Base._sub2ind(i[1].stop.I, i[2].I)

@generated function CartesianRange{N}(I::CartesianIndex{N})
startargs = fill(1, N)
:(CartesianRange($I($(startargs...)), I))
Expand Down
12 changes: 11 additions & 1 deletion test/arrayops.jl
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ for i = 1:3
end
@test isequal(a,findn(z))

#argmin argmax
#findmin findmax indmin indmax
@test indmax([10,12,9,11]) == 2
@test indmin([10,12,9,11]) == 3
@test findmin([NaN,3.2,1.8]) == (1.8,3)
Expand All @@ -316,6 +316,16 @@ end
@test findmin([3.2,1.8,NaN,2.0]) == (1.8,2)
@test findmax([3.2,1.8,NaN,2.0]) == (3.2,1)

# #14085
@test findmax(4:9) == (9,6)
@test indmax(4:9) == 6
@test findmin(4:9) == (4,1)
@test indmin(4:9) == 1
@test findmax(5:-2:1) == (5,1)
@test indmax(5:-2:1) == 1
@test findmin(5:-2:1) == (1,3)
@test indmin(5:-2:1) == 3

## permutedims ##

#keeps the num of dim
Expand Down

0 comments on commit 13a68a0

Please sign in to comment.