JuliaIntervals / TaylorModels.jl

Rigorous function approximation using Taylor models in Julia
Other
63 stars 15 forks source link

Ambiguity in broadcasted(+, ::IntervalBox, ::IntervalBox) #39

Closed schillic closed 5 years ago

schillic commented 5 years ago

(I was not sure whether to report in IntervalArithmetic.jl or here. Feel free to move around.)

Since a few days the following does not work anymore.

julia> using TaylorIntegration
julia> using TaylorModels: @taylorize
julia> using IntervalArithmetic: IntervalBox
julia> using TaylorModels: validated_integ

julia> @taylorize function vanderPol!(t, x, dx)
    local μ = 1.0
    dx[1] = x[2]
    dx[2] = (μ * x[2]) * (1 - x[1]^2) - x[1]
    return dx
end

julia> validated_integ(vanderPol!, [0., 1.], IntervalBox([0., 1.]), 0., 1., 2, 2, 1e-2)

ERROR: MethodError: Base.Broadcast.broadcasted(::typeof(+), ::IntervalBox{2,Float64}, ::IntervalBox{2,Float64}) is ambiguous. Candidates:
  broadcasted(f, X::IntervalBox, Y::IntervalBox) in IntervalArithmetic at .julia/packages/IntervalArithmetic/9q80F/src/multidim/arithmetic.jl:27
  broadcasted(f, X::IntervalBox, y) in IntervalArithmetic at .julia/packages/IntervalArithmetic/9q80F/src/multidim/arithmetic.jl:28
  broadcasted(f, x, Y::IntervalBox) in IntervalArithmetic at .julia/packages/IntervalArithmetic/9q80F/src/multidim/arithmetic.jl:29
  broadcasted(f::Union{typeof(*), typeof(+), typeof(muladd)}, arg1, arg2, args...) in DiffEqBase at .julia/packages/DiffEqBase/EFqMn/src/diffeqfastbc.jl:54
Possible fix, define
  broadcasted(::Union{typeof(*), typeof(+), typeof(muladd)}, ::IntervalBox, ::IntervalBox)
Stacktrace:
 [1] #validated_integ#31(::Int64, ::Bool, ::Function, ::Function, ::Function, ::Array{Float64,1}, ::IntervalBox{2,Float64}, ::Float64, ::Float64, ::Int64, ::Int64, ::Float64) at .julia/packages/TaylorModels/DXLdP/src/validatedODEs.jl:277
 [2] validated_integ(::Function, ::Array{Float64,1}, ::IntervalBox{2,Float64}, ::Float64, ::Float64, ::Int64, ::Int64, ::Float64) at .julia/packages/TaylorModels/DXLdP/src/validatedODEs.jl:265

It is quite mysterious because there were no changes in this package.

dpsanders commented 5 years ago

What is DiffEqBase doing in there? Is that relevant?

schillic commented 5 years ago

I don't think it is relevant, but who knows, and I did not investigate any further so far. I obtained the error message above in a fresh Julia session, so I guess one of the packages in the code above must have loaded that.

dpsanders commented 5 years ago

Ah, I guess TaylorIntegration loads it.

I have no idea what is going on here...

lbenet commented 5 years ago

Thanks for reporting!

What is DiffEqBase doing in there? Is that relevant?

DiffEqBase is used by TaylorIntegration.jl to a common interface (be callable from DiffEquations).

I can't reproduce the problem, i.e., everything runs fine in my set-up. Can you provide your current set-up (branches you are using)?

schillic commented 5 years ago

I use the release version of all the packages mentioned above in Julia v1.1. Our Travis build has the same error.

As I said above, it all worked fine until recently (precisely six days ago, see our build history). Maybe there was a problem with the change to General (I don't really think so, but it is really a strange thing).

schillic commented 5 years ago

We use + instead of .+ now in this place to avoid broadcasting (see here).

schillic commented 5 years ago

Another observation: Travis and I use Linux. @mforets uses a Mac and does not have this problem.

lbenet commented 5 years ago

For the record, I also work in a Mac and had no issue.

@schillic Does avoiding broadcasting solves the problem? In terms of performance, i do not know what is better in this case, but this can be easily fixed in TaylorModels.

lbenet commented 5 years ago

@dpsanders What about adding the possible fix to IntervalArithmetic.jl?

Possible fix, define
  broadcasted(::Union{typeof(*), typeof(+), typeof(muladd)}, ::IntervalBox, ::IntervalBox)
schillic commented 5 years ago

Does avoiding broadcasting solve the problem?

Yes, because there is no call to broadcasted (obviously :wink:).

mforets commented 5 years ago

As the error suggests, the problem is particular to IntervalArithmetic and is triggered if you load DiffEqBase (the function that creates the ambiguity is this one).

Consider the following minimal example:

julia> using IntervalArithmetic

julia> B = IntervalBox(0..1, 0..1);

julia> B .+ B
[0, 2] × [0, 2]

julia> using DiffEqBase # using v5.6.4 here

julia> B .+ B
ERROR: MethodError: Base.Broadcast.broadcasted(::typeof(+), ::IntervalBox{2,Float64}, ::IntervalBox{2,Float64}) is ambiguous. Candidates:
  broadcasted(f, X::IntervalBox, Y::IntervalBox) in IntervalArithmetic at /Users/forets/.julia/dev/IntervalArithmetic/src/multidim/arithmetic.jl:27
  broadcasted(f, X::IntervalBox, y) in IntervalArithmetic at /Users/forets/.julia/dev/IntervalArithmetic/src/multidim/arithmetic.jl:28
  broadcasted(f, x, Y::IntervalBox) in IntervalArithmetic at /Users/forets/.julia/dev/IntervalArithmetic/src/multidim/arithmetic.jl:29
  broadcasted(f::Union{typeof(*), typeof(+), typeof(muladd)}, arg1, arg2, args...) in DiffEqBase at /Users/forets/.julia/packages/DiffEqBase/EFqMn/src/diffeqfastbc.jl:54
Possible fix, define
  broadcasted(::Union{typeof(*), typeof(+), typeof(muladd)}, ::IntervalBox, ::IntervalBox)
Stacktrace:
 [1] top-level scope at none:0
dpsanders commented 5 years ago

So it seems like DiffEqBase is defining something too broad - in particular, it is committing type piracy by defining operations only on Base types.

mforets commented 5 years ago

Alright, and how should we proceed?

1) Report in DiffEqBase asking if DiffEqBase.broadcasted can be annotated with arg1 and/or arg2 as a DiffEqBC.

2) Try the possible fix broadcasted(::Union{typeof(*), typeof(+), typeof(muladd)}, ::IntervalBox, ::IntervalBox).

CC: @ChrisRackauckas

ChrisRackauckas commented 5 years ago

You're not supposed to annotate broadcasted args, only the ones with an array style. This is just replacing what's in Base with a patched version that should be coming to Julia soon anyways, fixing the aliasing issue. Why is broadcasted(f, X::IntervalBox, Y::IntervalBox) defined in the first place? You shouldn't be doing that.

Just like all the other machinery, broadcasted also computes and exposes the combined broadcast style of its arguments, so instead of specializing on broadcasted(f, args...), you can specialize on broadcasted(::DestStyle, f, args...) for any combination of style, function, and arguments.

https://docs.julialang.org/en/v1/manual/interfaces/#extending-in-place-broadcast-1

lbenet commented 5 years ago

This issue be at least partially fixed by https://github.com/PerezHz/TaylorIntegration.jl/pull/68.

Note also that https://github.com/JuliaDiffEq/DiffEqBase.jl/pull/211 was merged, where the exported broadcasted methods that caused the ambiguity were commented.

lbenet commented 5 years ago

https://github.com/PerezHz/TaylorIntegration.jl/pull/68 is now merged, and should have solved the problem reported here. Can you confirm @schillic ?

schillic commented 5 years ago

I still use the release version of TaylorIntegration, so the update there cannot have an influence for me. However, it still works now. The reason I guess is that the broadcasted overload in DiffEqBase got removed.

mforets commented 5 years ago

I think this can be closed. Thanks!

lbenet commented 5 years ago

Closing.