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.

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

specialize argmin/argmax on ranges #34315

Merged
merged 4 commits into from
Sep 29, 2020
Merged

Conversation

jw3126
Copy link
Contributor

@jw3126 jw3126 commented Jan 8, 2020

before:

julia> using BenchmarkTools; @btime argmin(1:1000)
  1.054 μs (0 allocations: 0 bytes)

after:

julia> using BenchmarkTools; @btime argmin(1:1000)
  1.901 ns (0 allocations: 0 bytes)

@jw3126
Copy link
Contributor Author

jw3126 commented Jan 15, 2020

merge?

@TotalVerb
Copy link
Contributor

TotalVerb commented Jan 16, 2020

This is not something to care about (will not break any actual code), but it may be worth mentioning that argmax currently will return the first of all maximal elements, and elements in Float ranges need not be unique. For example,

julia> argmax(9.999999999e15:1e16)
1000000

julia> lastindex(9.999999999e15:1e16)
1000001

Nevertheless I think this specialization makes sense. It might be worth documenting that the "If there are multiple maximal elements, then the first one will be returned." does not necessarily apply to certain pathological ranges, but then again, perhaps this case is so rare that it's unimportant. (Or, maybe revise the current argmax documentation to say "For Arrays, if there are multiple [...]", which gives implementors of custom types freedom to choose a different extremum if simpler.)

@jw3126
Copy link
Contributor Author

jw3126 commented Jan 16, 2020

Urgs, I think it is nice that argmax returns the first maximum in general. Another way out would be to add a slow branch that does binary search on ranges in case that e.g. the previous element of the last one is as big.

@jw3126
Copy link
Contributor Author

jw3126 commented Jan 16, 2020

Here is an example of a degenerate range, that I can imagine happening in real code:

julia> r = map(Float32, 1:10^8)
1.0f0:1.0f0:1.0f8

julia> collect(r[end-10:end])
11-element Array{Float32,1}:
 9.999999e7
 9.999999e7
 9.999999e7
 9.999999e7
 9.999999e7
 1.0e8     
 1.0e8     
 1.0e8     
 1.0e8     
 1.0e8     
 1.0e8     

@jw3126
Copy link
Contributor Author

jw3126 commented Jan 16, 2020

Here is a related bug:

julia> searchsorted(1f0:1f8, 1f8)
100000000:100000000
julia> (1f0:1f8)[end-1]
1.0f8

@jw3126
Copy link
Contributor Author

jw3126 commented Jan 16, 2020

function argmin(r::AbstractRange)
    if isempty(r)
        throw(ArgumentError("range must be non-empty"))
    elseif step(r) > 0
        firstindex(r)
    else
        first(searchsorted(r, last(r)))
    end
end

This should be the solution if there was not #34408 . I don't feel like fixing #34408 however, at least not any time soon. So I documented, that a non first index may be returned.

@jw3126
Copy link
Contributor Author

jw3126 commented Jan 21, 2020

I think CI fail is unrelated to the PR.

@StefanKarpinski
Copy link
Sponsor Member

Can you rebase on a newer master? That's a lot of test failures, which makes me nervous.

jw3126 and others added 2 commits January 21, 2020 23:23
Co-Authored-By: Stefan Karpinski <stefan@karpinski.org>
@jw3126
Copy link
Contributor Author

jw3126 commented Sep 25, 2020

Can somebody rerun the failing CI?

@StefanKarpinski StefanKarpinski merged commit 24c468d into JuliaLang:master Sep 29, 2020
@StefanKarpinski
Copy link
Sponsor Member

That particular CI failure has been happening lately and is unrelated. Very unlikely that this is failing only on FreeBSD, so I just merged since there were no other failures there or elsewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants