StatsBase.jl icon indicating copy to clipboard operation
StatsBase.jl copied to clipboard

Method ambiguities on 0.7

Open ararslan opened this issue 7 years ago • 8 comments

Seen on Travis:

Standard Deviation: Error During Test at /home/travis/.julia/v0.7/StatsBase/test/moments.jl:27
  Test threw an exception of type MethodError
  Expression: std(x, wv; corrected=false) ≈ expected_std
  MethodError: getfield(Base, Symbol("#kw##std"))()(::NamedTuple{(:corrected,),Tuple{Bool}}, ::typeof(std), ::Array{Float64,1}, ::Weights{Float64,Float64,Array{Float64,1}}) is ambiguous. Candidates:
    (::getfield(Base, Symbol("#kw##std")))(::Any, ::typeof(std), v::AbstractArray{T,N} where N where T<:Real, w::AbstractWeights) in StatsBase
    (::getfield(Base, Symbol("#kw##std")))(::Any, ::typeof(std), A::AbstractArray{#s538,N} where N where #s538<:AbstractFloat, region) in Base
  Possible fix, define
    (::getfield(Base, Symbol("#kw##std")))(::Any, ::typeof(std), ::AbstractArray{#s538,N} where N where #s538<:AbstractFloat, ::AbstractWeights)
  Stacktrace:
   [1] macro expansion at /home/travis/.julia/v0.7/StatsBase/test/moments.jl:27 [inlined]
   [2] macro expansion at /buildworker/worker/package_linux64/build/usr/share/julia/site/v0.7/Test/src/Test.jl:1008 [inlined]
   [3] macro expansion at /home/travis/.julia/v0.7/StatsBase/test/moments.jl:27 [inlined]
   [4] macro expansion at /buildworker/worker/package_linux64/build/usr/share/julia/site/v0.7/Test/src/Test.jl:1080 [inlined]
   [5] macro expansion at /home/travis/.julia/v0.7/StatsBase/test/moments.jl:13 [inlined]
   [6] macro expansion at /buildworker/worker/package_linux64/build/usr/share/julia/site/v0.7/Test/src/Test.jl:1008 [inlined]
   [7] top-level scope at /home/travis/.julia/v0.7/StatsBase/test/moments.jl:6

That's one of a few things that are failing, all related.

ararslan avatar Feb 10 '18 19:02 ararslan

I'm confused... what's changed in base? The method signature it's claiming is ambiguous seems strictly more specific than the candidates.

rofinn avatar Feb 11 '18 17:02 rofinn

It's because of the region argument that is untyped (ambiguous w/ the AbstractWeights arg).

quinnj avatar Feb 11 '18 17:02 quinnj

May be fixed by https://github.com/JuliaLang/julia/pull/25989 in 1.0 once the deprecation is removed? I would say that we could consider similarly making weights a keyword argument, but I'm not sure it's possible to extend Base methods that way.

ararslan avatar Feb 11 '18 20:02 ararslan

I would say that we could consider similarly making weights a keyword argument, but I'm not sure it's possible to extend Base methods that way.

No, it's not possible as keyword arguments don't participate in dispatch.

AFAICT we just need to add a more specific definition to fix the ambiguity, right?

nalimilan avatar Feb 12 '18 09:02 nalimilan

@rofinn Could https://github.com/JuliaLang/julia/pull/25832 have caused this?

andreasnoack avatar Feb 12 '18 09:02 andreasnoack

Ahh, right, I didn't notice the region argument. Seems like the region should just be typed as Integer in base regardless. Also, is there a reason the base function only works on AbstractFloats (vs Real)? @andreasnoack I'm not familiar enough with the dispatch code in base to know if that's what's causing this. I didn't even realize keyword args show up as a NamedTuple now :/

rofinn avatar Feb 12 '18 16:02 rofinn

I have seen quite a bit of legacy code in StatsBase.jl. Would a Julia 0.7-rc only version re-write be desirable? For example,

return cm3 / sqrt(cm2 * cm2 * cm2)  # this is much faster than cm2^1.5

which is no longer the case since a few releases back. If so, I could work on it once Julia 0.7-rc1 is out.

Nosferican avatar Mar 12 '18 03:03 Nosferican

If you can identify places where the code could be made significantly simpler on 0.7, I guess we can use if VERSION > ... conditionals so that Femtocleaner automatically removes the old one once we require 0.7. But the first step would probably be supporting 0.7 without deprecation warnings...

nalimilan avatar Mar 12 '18 08:03 nalimilan