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

Method ambiguities on 0.7 #346

Open
ararslan opened this issue Feb 10, 2018 · 8 comments
Open

Method ambiguities on 0.7 #346

ararslan opened this issue Feb 10, 2018 · 8 comments
Labels

Comments

@ararslan
Copy link
Member

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.

@rofinn
Copy link
Member

rofinn commented Feb 11, 2018

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

@quinnj
Copy link
Member

quinnj commented Feb 11, 2018

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

@ararslan
Copy link
Member Author

May be fixed by JuliaLang/julia#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.

@nalimilan
Copy link
Member

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?

@andreasnoack
Copy link
Member

@rofinn Could JuliaLang/julia#25832 have caused this?

@rofinn
Copy link
Member

rofinn commented Feb 12, 2018

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 :/

@Nosferican
Copy link
Contributor

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.

@nalimilan
Copy link
Member

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...

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

No branches or pull requests

7 participants