Loose signature of push! fallback (results in confusing MethodError)

[samuelpowell]

I am struggling to find a minimal test case for this problem.

The following code:

using Optim

function f_gd(x)
  Float32((x[1] - 5.0)^2)
end

function g_gd(x, storage)
  storage[1] = 2.0 * (x[1] - 5.0)
end

initial_x = [0.0]

d = DifferentiableFunction(f_gd, g_gd)

results = Optim.gradient_descent(d, initial_x)

Fails with the following error:

ERROR: MethodError: `push!` has no method matching push!(::Optim.LineSearchResults{Float64}, ::Float64)
Closest candidates are:
  push!{T}(::Optim.LineSearchResults{T}, ::T, ::T, ::T)
  push!(::Any, ::Any, ::Any)
  push!(::Any, ::Any, ::Any, ::Any...)
  ...
 in push! at abstractarray.jl:1365
 in gradient_descent at /Users/samuelpowell/.julia/v0.4/Optim/src/gradient_descent.jl:90

The error suggests that push! is being called with two arguments, but in fact, the offending line calls push! correctly with 4 arguments: push!(lsr, zero(T), f_x, dphi0).

An error should be thrown, since I have forced f_x to single precision (it is the output of function f_gd(x)), but this should have read:

ERROR: MethodError: `push!` has no method matching push!(::Optim.LineSearchResults{Float64}, ::Float64, ::Float32, ::Float64)

For some reason, the last two argument types are missing.

I can reproduce this behaviour on a 0.4-dev and a 0.4-pre on three different architectures (including JuliaBox)

[johnmyleswhite]

I hit something like this recently. There's some implicit redirection of one push! to another push! method, but I forget quite where it happens.

[yuyichao]

The one that causes the MethodError is called with 2 arguments. You can see that in the backtrace

[yuyichao]

The fallback method is printed in the backtrace in push! at abstractarray.jl:1365 so you should be able to figure out why that happens. I'm not sure what we can do to improve this other than making sure inlined functions don't mess up the backtrace https://github.com/JuliaLang/julia/pull/12544

[johnmyleswhite]

One thing we could do here is to remove push!(::Any, ::Any, ::Any) and their kind, forcing people to use push!(Any, Tuple{Any}) instead. Since I've hit this exact issue myself, I'd argue that push!(::Any, ::Any, ::Any) might be excessively generic -- it implicitly enrolls ever single type in Julia into a contract that says that push!(x, y, z) can always be reinterpreted as push!(push!(x, y), z).

[johnmyleswhite]

Just to make the tradeoffs clear, the alternative perspective is that push!(::Optim.LineSearchResults{Float64}, ::Float64, ::Float64, ::Float64) is excessively specific, so reasonable arguments end up failing to match that signature, but instead end getting matched by push!(Any, Any, Any, Any) instead.

[yuyichao]

I don't think push!(::Any, ::Tuple{Vararg{Any}}) would work because it would be anbigious with normal normal push!. Maybe limit the vararg definition to push(::AbstractArray, ::Any...)?

[johnmyleswhite]

Yeah, I think enforcing an AbstractArray solution is good, although it might need to be something like Union{AbstractArray, Associative} since we allow using push! to mutate dicts.

[samuelpowell]

@johnmyleswhite, @yuyichao, thank you for looking into this. I understand now what was happening, and as you say, this is pretty obvious when looking at the push! methods in abstractarray.jl. I was thrown off guard by the error message.

I have updated the issue accordingly.

The (::Optim.LineSearchResults{T}, ::T, ::T, ::T) method signature, whilst specific, is reasonably sane: in most cases one would expect an objective function and its derivative to be of the same type.

The suggested tightening of the signature in Base seems like a good solution, providing it doesn't cause breakage.