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

Merging max and maximum #11872

Closed
dphildebrandt opened this issue Jun 26, 2015 · 7 comments
Closed

Merging max and maximum #11872

dphildebrandt opened this issue Jun 26, 2015 · 7 comments
Labels
won't change Indicates that work won't continue on an issue or pull request

Comments

@dphildebrandt
Copy link

Related to #4235 and #4652 it seems like it would be nice to have one keyword
for all cases, preferably max instead of max and maximum.

The main reason we can't combine the two is because the following methods
would collide under the same function name:

max{T1<:Real,T2<:Real}(::AbstractArray{T1<:Real,N}, ::T2<:Real) at operators.jl:378
maximum(A::AbstractArray{T,N}, region) at reducedim.jl:261

Proposal:

Change max to handle all of maximum's methods, discarding maximum and
discarding the max method mentioned above.

Instead, this aforementioned max method can be achieved through several
options:

  1. broadcast (or map?) max
  2. create a .max function (if possible?) in the same vein as .+, etc.
  3. create a new keyword for a vectorized function with the same method

This is partly related to @johnmyleswhite's talk on 2015-06-25 JuliaCon
which mentioned vectorizing operations might be an issue and we should possibly
move back to using map.

Also #4235 (comment) mentioned map was too slow, is this still the case with broadcast?

@tkelman
Copy link
Contributor

tkelman commented Jun 26, 2015

Is that really the main ambiguity driving the distinction? Perhaps max{T1<:Real,T2<:Real}(A::AbstractArray{T1<:Real,N}, b::T2<:Real) could be deprecated in favor of clamp(A, b, typemax(T2)), though I did have to think about which way that needs to go for longer than I'd like to admit, and I would likely get it backwards way too often.

@dphildebrandt dphildebrandt changed the title Merging max and minimum Merging max and maximum Jun 26, 2015
@dphildebrandt
Copy link
Author

Apparently there is a more generalized issue for this problem #8450 @JeffBezanson mentioned in his JuliaCon talk today. Good to see others aren't crazy about the vectorized versions of functions.

@johnmyleswhite
Copy link
Member

I think this depends on mapslices more than map

@JeffBezanson
Copy link
Sponsor Member

maximum is a reduction, so the right thing would be to use some combination of max, reduce, and mapslices.

@stevengj
Copy link
Member

Note that in principle we could revisit this now. max.(a,b) does the element-wise thing, and element-wise max(a,b) is deprecated in #17302. So that frees up max(a) to do reduction again. Per our usual policy of "one release cycle of deprecation", however, the latter couldn't happen until Julia 0.7.

@eschnett
Copy link
Contributor

eschnett commented Sep 1, 2016

There's an ambiguity here. If there is a type T that is both ordered and is a collection, then what should max(x::T) do? Should it apply to x as a collection (and return its maximum), or should it apply to x as ordered type (and return x)?

Compare to + and sum. + with a single argument returns that argument unchanged, sum with a single argument treats it as collection and sums its elements. max and + behave very similarly in this respect.

A real-world case are SIMD vectors. For these, both max and maximum is well-defined, and they return different results.

@JeffBezanson JeffBezanson added the won't change Indicates that work won't continue on an issue or pull request label Apr 8, 2017
@JeffBezanson
Copy link
Sponsor Member

I don't think max and reduce(max, x) can be considered the same function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
won't change Indicates that work won't continue on an issue or pull request
Projects
None yet
Development

No branches or pull requests

6 participants