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

problematic max and/or splatting #9439

Closed
waTeim opened this issue Dec 22, 2014 · 18 comments
Closed

problematic max and/or splatting #9439

waTeim opened this issue Dec 22, 2014 · 18 comments
Labels
docs This change adds or pertains to documentation

Comments

@waTeim
Copy link
Contributor

waTeim commented Dec 22, 2014

julia> help("max")
Base.max(x, y, ...)

   Return the maximum of the arguments. Operates elementwise over
   arrays.

Ok trying it....

julia> @time max(rand(30000))
ERROR: `max` has no method matching max(::Array{Float64,1})

Ok, maybe splatting?

julia> @time max(rand(30000)...)
elapsed time: 6.477102746 seconds (3602294488 bytes allocated, 27.43% gc time)
0.9999923243300144
                                        ^  LoLwut?

In comparison

julia> @time maxabs(rand(30000))
elapsed time: 0.001348913 seconds (240184 bytes allocated)
0.999986768847797

Also

julia> @time maxabs(rand(100)...)
ERROR: maxabs has no method matching maxabs(::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64)

The reason I pasted this is because it took a long time for this to return the first time but then was very fast the next time for a whole bunch of different values less than 100, so I think it's probably trying to compile for all possibilities up to 100.

julia> methods(maxabs)
#2 methods for generic function "maxabs":
maxabs(A::AbstractArray{T,N},region) at reducedim.jl:243
maxabs(a) at reduce.jl:317

julia> methods(max)
#10 methods for generic function "max":
max(x::Base.MPFR.BigFloat,y::Base.MPFR.BigFloat) at mpfr.jl:497
max{T<:FloatingPoint}(x::T<:FloatingPoint,y::T<:FloatingPoint) at math.jl:191
max{T<:Real}(x::T<:Real,y::T<:Real) at promotion.jl:201
max(x::Real,y::Real) at promotion.jl:181
max{T1<:Real,T2<:Real}(::T1<:Real,::AbstractArray{T2<:Real,N}) at operators.jl:370
max{T1<:Real,T2<:Real}(::AbstractArray{T1<:Real,N},::T2<:Real) at operators.jl:372
max{T1<:Real,T2<:Real}(::AbstractArray{T1<:Real,N},::AbstractArray{T2<:Real,N}) at operators.jl:376
max(x,y) at operators.jl:56
max(a,b,c) at operators.jl:83
max(a,b,c,xs...) at operators.jl:84

Pretty up-to-date

julia> versioninfo()
Julia Version 0.4.0-dev+2177
Commit a2ff1ea (2014-12-17 01:05 UTC)
Platform Info:
  System: Darwin (x86_64-apple-darwin13.4.0)
  CPU: Intel(R) Core(TM) i7-4850HQ CPU @ 2.30GHz
  WORD_SIZE: 64
  BLAS: libopenblas (USE64BITINT DYNAMIC_ARCH NO_AFFINITY Haswell)
  LAPACK: libopenblas
  LIBM: libopenlibm
  LLVM: libLLVM-3.3
@andreasnoack
Copy link
Member

You are looking for maximum which is the reduction version of max. The help message for max explains that it takes at least two arguments. However, the naming of maxabs seems unfortunate. I think it would have to be maximumabs.

@nalimilan
Copy link
Member

I'm still not convinced the max vs. maximum distinction is clear enough.

@timholy
Copy link
Member

timholy commented Dec 22, 2014

And the help is outright lying; @waTeim, care to submit a documentation fix?

@ivarne
Copy link
Member

ivarne commented Dec 22, 2014

The doc isn't lying! it's just misleading for people that doesn't understand the terminology. max works elementwise just like + works elementwise. If you ask for +(rand(3000)), you get a different result from sum(rand(3000)), because the first works elementwise, while the second does a reduction.

The distinction is pretty subtle, but important, because of multiple dispatch and our habit to be able to do reduction operations with an optional region or dims parameter, to do reduction in one direction in a multidimensional array. If we don't make that extra parameter a keyword argument, we will get ambiguities for max(::AbstractArray,::AbstractArray).

@johnmyleswhite
Copy link
Member

because the first works elementwise, while the second does a reduction.

This isn't a very meaningful distinction. What you're referring to is the ordering of operations in addition, which is what both +(rand(3000)) and sum(rand(3000)) achieve. One does the addition left-to-right and the other involves splitting the work and then aggregating the results of sub-sums. Both achieve a "reduction".

@timholy
Copy link
Member

timholy commented Dec 22, 2014

You're right, of course. But certainly this is a good example of our documentation being too terse; it would be really easy to interpret that as meaning that max is doing what maximum does.

@nalimilan
Copy link
Member

I wonder whether we might be able to get rid of min/max, replacing them with a broader element-wise operations framework (cf. #8450).

@timholy
Copy link
Member

timholy commented Dec 22, 2014

@johnmyleswhite, I think @ivarne meant this:

julia> max([1,5,2],[3,2,2])
3-element Array{Int64,1}:
 3
 5
 2

@johnmyleswhite
Copy link
Member

Ah, thanks @timholy.

@StefanKarpinski
Copy link
Member

@johnmyleswhite, +(rand(3000)) just returns a 3000-element random vector sum(rand(3000)) returns the sum of such a vector, so they're pretty different. I'm still not entirely convinced that this was the right API choice because I always try max first and then use maximum only when that fails. This may be the fault of my brain being trained to think of these as the same operation when they're not for so long.

@ivarne
Copy link
Member

ivarne commented Dec 22, 2014

Thanks @timholy. I should've made it clear that +(::Array) is the identity function, and that one might argue that max should be the same. (Currently it isn't and that's good because the identity function is not worth much and there is huge potential for confusion).

I had huge problems understanding the difference between max and maximum when the separation was made, so I certainly don't blame anyone for being confused.

I don't see how we can get rid of the elementwise max and min functions in light of #8450. We would still need a different function to be the base of max(a, b) = a<b ? b : a, that you would generalize to a map elementwise function with the syntax from #8450.

@andreasnoack
Copy link
Member

Some examples and a cross reference to maximum in the documentation would be very useful and probably remove some of the confusion here.

A faster reduce would probably make maximum redundant.

@nalimilan
Copy link
Member

@ivarne Right, that was a silly idea. That's really more a naming issue than anything else.

@waTeim
Copy link
Contributor Author

waTeim commented Dec 22, 2014

Ok, got it, I'll play the roll of user here, provide some feedback

  1. There's both max AND maximum?
    That's just asking for confusion. I understand there was a split that happened for whatever reason it happened, but is there really any danger of misunderstanding? It seems like the nature of the arguments would lead to a pretty clear expectation. When I see max is an actual function, I stop looking for anything else. After all what implicit meaning does imum have? That said when I tried to use max(Array) and it didn't work, I did turn to the documentation. A semi-lengthy explanation explicitly mentioning maxiumum as an alternative with some nice examples of both would have set me straight. If you're asking me to do that, yea, I can do that.

  2. All good, but how come max(rand(30000)...) is 10000x slower and allocates 3GB of memory?
    So I definitely used the wrong function, but it's legal, it computes the same thing and it takes 1 less character to type ...

    julia> @time maximum(rand(30000))
    elapsed time: 0.000408836 seconds (240184 bytes allocated)
    0.9999612972082188

  3. It took me a while to read all of Alternative syntax for map(func, x) #8450, here's my reaction
    It was lengthy and looks like the implementation will take a while, and if both max and maxiumum must exist until that occurs, I'm cool with that. Whatever the solution I don't feel the need for a lot of fine-grain control, but I would like to not have to type a bunch. I like the idea of underpinning things with lazy evaluation. Some suggestions I thought too wordy .operator is ok but .identifier or identifier. seems strange because the rest of the internet uses it as an indicator of element components or specialization (e.g. domain names, record fields, class members). I didn't have a problem with identifier[] and I'd also be ok with f->(tuple expr), while realizing that ambiguity would have to be resolved.

@nalimilan
Copy link
Member

@waTeim Regarding your points:

  1. See min/max reductions are confusing #4235 I'm not a fan of the names, but at least you'll have the history of the current design. Improving the docs is certainly a good idea (even if a better solution can be found in the future).
  2. Splatting a large array will always be slow, and it's hard to forbid since it would mean max(a, b) works for arrays and not for scalars. I don't think it's a big issue: if we could find more explicit names for the functions in the first place, people would never try this kind of weird call.
  3. Honestly I'm really not sure my proposal about this makes sense.

@tknopp
Copy link
Contributor

tknopp commented Dec 23, 2014

I think we really should take the max/maximum issue as an example where we/Julia reach limits and learn from it how to proceed. Documentation is nice but lets be honest. The documentation will be there because of unintuitive naming.

I don't have a solution but my understanding is that we will not be able to use a single function name for the two purposes. What about removing max entirely? And call one max_of_args and the other max_of_array? These names are certainly not good but I hope one gets the idea.

@nalimilan
Copy link
Member

pmax/pmin didn't sound like a bad name. Not sure why it wasn't retained in #4235, as people seemed to like it.

@ivarne ivarne added the docs This change adds or pertains to documentation label Feb 27, 2015
@KristofferC
Copy link
Member

The distinction seems pretty clear now:

See also the maximum function to take the maximum element from a collection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation
Projects
None yet
Development

No branches or pull requests

9 participants