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

Add a by keyword to maximum and minimum #28210

Open
davidanthoff opened this issue Jul 20, 2018 · 8 comments
Open

Add a by keyword to maximum and minimum #28210

davidanthoff opened this issue Jul 20, 2018 · 8 comments
Labels
feature Indicates new feature / enhancement requests

Comments

@davidanthoff
Copy link
Contributor

Essentially the same thing that already exists for sort: an option to specify a selector function, so that the maximum is determined by whatever the selector function returns, but then the maximum function returns an element from the original input sequence.

For example, maximum([(a=1,b=2),(a=2,b=1)], by=i->i.b) would return (a=1,b=2).

@StefanKarpinski
Copy link
Member

Makes sense—any ordering-dependent function should support this kind of API.

@davidanthoff
Copy link
Contributor Author

Just saw #27639, where argmax(f, v) was suggested as an alternative.

@stevengj
Copy link
Member

See also #19190 and #19359.

@sam0410
Copy link
Contributor

sam0410 commented Dec 17, 2018

Is it fine if we write a function like this ?

function ($fname)(a::AbstractArray; dims=:, by=identity)
    val = ($_fname)(by, a, dims)
    for x in a
        if by(x) == val
            return x
        end
    end
end

@giordano
Copy link
Contributor

A solution could be adding a by keyword argument to isless and then forwarding this keyword argument from maximum/minimum all the way down to isless, right?

E.g.

julia> isless((1,2),(2,1))
true

julia> function Base.isless(t1::Tuple, t2::Tuple; by = first)
           a, b = by(t1), by(t2)
           isless(a, b) || (isequal(a, b) && isless(tail(t1), tail(t2)))
       end

julia> isless((1,2),(2,1))
true

julia> isless((1,2),(2,1), by = i->i[2])
false

@chethega
Copy link
Contributor

Any implementation that is compatible with Base.Order has minimum([NaN,0.1, 0.2, missing]; by = identity) == 0.1, which is notably different from minimum([NaN,0.1, 0.2, missing]) == missing, and also minimum(Any[0, 1.0]) isa Float64 while minimum(Any[0, 1.0]; by=identity) isa Int64.

I am in favor of using the Base.Order framework. This means that we need to duplicate some code: The usual min/max comparison is in an entirely different category than isless.

@tkluck
Copy link
Contributor

tkluck commented Feb 12, 2020

Reviving this one as its related to #34719 .

Any implementation that is compatible with Base.Order has minimum([NaN,0.1, 0.2, missing]; by = identity) == 0.1

Here's another way to think about it. Let's just restrict attention to min(a, b) as minimum(...) should just be equal to reduce(min, ...), with keywords passed through.

An important reason why min(::Float64, ::Int) isa Float64 is type stability. But when we extend with keywords, we can't really expect type stability in every case; for example,

min((a=2, b=4), (a=5.0, b=2), by=x -> x.b)

or even

min((a=2,b=4), (a=4.0,b=2,c=6), by=x -> x.b)

is never going to have a sane type-stable interpretation. I see two options:

  • Make min(::Number, ::Number; kwds...) type-stable through promotion even when by= is passed.
  • Make it type-stable whenever promote allows it: something like
function min(a, b; lt=isless, by=identity, order=Forward)
    if isconcretetype(promote_type(typeof(a), typeof(b)))
        a, b = promote(a, b)
    end
    isordered = Base.Order.lt(_ord(lt, by, order), a, b)
    return isordered ? a : b
end

Julia will evaluate isconcretetype(...) at compile-time and collapse the branch. It also doesn't depend on inferred types so it's API stable. It's also backwards compatible with current min(a, b) behaviour with the default keyword arguments.

I have a preference for this solution because I'm a heavy user of non-Number types that do support promote. Any thoughts/objections before I make a pull request?

@jariji
Copy link
Contributor

jariji commented Oct 3, 2022

findmax needs by= too. For example:

> passnothing(f) = x->isnothing(x) ? nothing : f(x)
> findmax(passnothing(abs), [-2., -4., nothing]; by=x->isnothing(x) ? -Inf : x) == (4., 2)

@brenhinkeller brenhinkeller added the feature Indicates new feature / enhancement requests label Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Indicates new feature / enhancement requests
Projects
None yet
Development

No branches or pull requests

9 participants