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

Fix argmin and argmax #98

Merged
merged 8 commits into from
May 10, 2024
Merged

Fix argmin and argmax #98

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0ba1041
Fix argmin and argmax
gdalle May 10, 2024
9851910
Revert bqd fix
gdalle May 10, 2024
628839b
Add two small tests
gdalle May 10, 2024
01396a1
Moar tests to figure out what's wrong
gdalle May 10, 2024
64f2a78
Fix tests
gdalle May 10, 2024
5dcebff
Fix comparison order
gdalle May 10, 2024
926a899
last fix
gdalle May 10, 2024
0178dde
One comp only
gdalle May 10, 2024
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
9 changes: 5 additions & 4 deletions src/chainedvector.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -825,8 +825,9 @@ function findXwithfirst(comp, f, x, y, i)
for A in x.arrays
for y′ in A
y′′ = f(y′)
y = ifelse(comp(y′′, y), y′′, y)
i = ifelse(comp(y′′, y), i′, i)
c = comp(y′′, y) # store this before y changes
y = ifelse(c, y′′, y)
i = ifelse(c, i′, i)
i′ += 1
end
end
Expand All @@ -835,8 +836,8 @@ end

Base.findmax(x::ChainedVector) = findmax(identity, x)
Base.findmin(x::ChainedVector) = findmin(identity, x)
Base.argmax(x::ChainedVector) = findmax(identity, x)[1]
Base.argmin(x::ChainedVector) = findmin(identity, x)[1]
Base.argmax(x::ChainedVector) = findmax(identity, x)[2]
Base.argmin(x::ChainedVector) = findmin(identity, x)[2]
Base.argmax(f::F, x::ChainedVector) where {F} = x[findmax(f, x)[2]]
Base.argmin(f::F, x::ChainedVector) where {F} = x[findmin(f, x)[2]]

Expand Down
11 changes: 11 additions & 0 deletions test/chainedvector.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,17 @@
@test all(==(2), x)
@test length(x) == 5

# https://github.com/JuliaData/SentinelArrays.jl/issues/97
x = ChainedVector([[18, 70, 92, 15, 65], [25, 14, 95, 54, 57]])
@test findmax(x) == (95, 8)
@test findmin(x) == (14, 7)
@test argmax(x) == 8
@test argmin(x) == 7
@test findmax(inv, x) == (inv(14), 7)
@test findmin(inv, x) == (inv(95), 8)
@test argmax(inv, x) == 14
@test argmin(inv, x) == 95
Comment on lines +289 to +296
Copy link

Choose a reason for hiding this comment

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

Suggested change
@test findmax(x) == (95, 8)
@test findmin(x) == (14, 7)
@test argmax(x) == 8
@test argmin(x) == 7
@test findmax(inv, x) == (inv(14), 7)
@test findmin(inv, x) == (inv(95), 8)
@test argmax(inv, x) == 14
@test argmin(inv, x) == 95
@test let (val, idx) = findmax(x)
max_val = maximum(x)
val == max_val == x[idx]
end
@test let (val, idx) = findmin(x)
min_val = minimum(x)
val == min_val == x[idx]
end
@test x[argmax(x)] == maximum(x)
@test x[argmin(x)] == minimum(x)
@test let (val, idx) = findmax(inv, x)
max_val = maximum(inv, x)
val == max_val == inv(x[idx])
end
@test let (val, idx) = findmin(inv, x)
min_val = minimum(inv, x)
val == min_val == inv(x[idx])
end
@test x[argmax(inv, x)] == maximum(inv, x)
@test x[argmin(inv, x)] == minimum(inv, x)

IMO writing it like this would be a bit more robust against changing x in the future and forgetting to change the hardcoded data. This also ends up testing that argmax actually has to return an index, and not just the right number for this test case :)

Copy link
Member

Choose a reason for hiding this comment

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

I'll adopt this in #99


x = ChainedVector(Vector{Float64}[])
@test !any(x -> iseven(x), x)
@test !any(map(x -> iseven(x), x))
Expand Down
Loading