JuliaRobotics / IncrementalInference.jl

Clique recycling non-Gaussian (multi-modal) factor graph solver; also see Caesar.jl.
MIT License
72 stars 21 forks source link

iterate(::Nothing) induced by new NLsolve v3.0.0 #183

Closed dehann closed 5 years ago

dehann commented 6 years ago

adopting issue defined in JuliaLang/METADATA.jl#19282

Local testing breaks in a newly created test file here (error below): https://github.com/JuliaRobotics/IncrementalInference.jl/blob/5065fdfcc0171bf5a8ef977c9bd0df598aa9e138/test/testNLsolve.jl#L60

cc @pkofod -- thanks for taking a look.

I'm little at a loss as to what is happening. I am using NLsolve in a functional way: Not just passing one function to nlsolve but actually programmatically building the required function and passing that ccw = CommonConvWrapper wrapper function to nlsolve(ccw, x0). ccw then internally does a few things and calls a function defined by the user with a much larger parameter list, that is in ccw.usrfnc!. This all works fine in NLsolve v2.1.0.

MethodError: no method matching iterate(::Nothing)
Closest candidates are:
  iterate(!Matched::Core.SimpleVector) at essentials.jl:589
  iterate(!Matched::Core.SimpleVector, !Matched::Any) at essentials.jl:589
  iterate(!Matched::ExponentialBackOff) at error.jl:171
  ...
copyto!(::Array{Float64,1}, ::Nothing) at abstractarray.jl:646
(::getfield(NLsolve, Symbol("#f!#1")){CommonConvWrapper{OneDimensionTest{Normal{Float64}}}})(::Array{Float64,1}, ::Array{Float64,1}) at helpers.jl:6
(::getfield(NLSolversBase, Symbol("#fj!#19")){getfield(NLsolve, Symbol("#f!#1")){CommonConvWrapper{OneDimensionTest{Normal{Float64}}}},DiffEqDiffTools.JacobianCache{Array{Float64,1},Array{Float64,1},Array{Float64,1},Val{:central},Float64,Val{true}}})(::Array{Float64,1}, ::Array{Float64,2}, ::Array{Float64,1}) at oncedifferentiable.jl:93
value_jacobian!!(::OnceDifferentiable{Array{Float64,1},Array{Float64,2},Array{Float64,1}}, ::Array{Float64,1}, ::Array{Float64,2}, ::Array{Float64,1}) at interface.jl:130
value_jacobian!! at interface.jl:128 [inlined]
trust_region_(::OnceDifferentiable{Array{Float64,1},Array{Float64,2},Array{Float64,1}}, ::Array{Float64,1}, ::Float64, ::Float64, ::Int64, ::Bool, ::Bool, ::Bool, ::Float64, ::Bool, ::NLsolve.NewtonTrustRegionCache{Array{Float64,1}}) at trust_region.jl:119
trust_region at trust_region.jl:229 [inlined]
trust_region at trust_region.jl:229 [inlined]
#nlsolve#14(::Symbol, ::Float64, ::Float64, ::Int64, ::Bool, ::Bool, ::Bool, ::Static, ::Float64, ::Bool, ::Int64, ::Float64, ::typeof(nlsolve), ::OnceDifferentiable{Array{Float64,1},Array{Float64,2},Array{Float64,1}}, ::Array{Float64,1}) at nlsolve.jl:23
#nlsolve at nlsolve.jl:0 [inlined]
#nlsolve#15 at nlsolve.jl:60 [inlined]
nlsolve(::Function, ::Array{Float64,1}) at nlsolve.jl:50
top-level scope at none:0
include_string(::Module, ::String, ::String) at loading.jl:1005
include_string(::Module, ::String, ::String, ::Int64) at eval.jl:30
(::getfield(Atom, Symbol("##114#119")){String,Int64,String})() at eval.jl:94
withpath(::getfield(Atom, Symbol("##114#119")){String,Int64,String}, ::String) at utils.jl:30
withpath at eval.jl:46 [inlined]
#113 at eval.jl:93 [inlined]
with_logstate(::getfield(Atom, Symbol("##113#118")){String,Int64,String}, ::Base.CoreLogging.LogState) at logging.jl:397
with_logger at logging.jl:493 [inlined]
#112 at eval.jl:92 [inlined]
hideprompt(::getfield(Atom, Symbol("##112#117")){String,Int64,String}) at repl.jl:85
macro expansion at eval.jl:91 [inlined]
(::getfield(Atom, Symbol("##111#116")))(::Dict{String,Any}) at eval.jl:86
handlemsg(::Dict{String,Any}, ::Dict{String,Any}) at comm.jl:164
(::getfield(Atom, Symbol("##19#21")){Array{Any,1}})() at task.jl:259
dehann commented 6 years ago

similarly, see Travis error: https://travis-ci.org/JuliaRobotics/IncrementalInference.jl/jobs/451606069

pkofod commented 6 years ago

Right, so this is something I actually did anticipate, but I've earlier been told it's not a problem people who do similar things have experienced. However, we can fix this!

In NLsolve >=3.0.0, nlsolve(f, x) will automatically detect if f has a single input method. The contract here is that if you have a method residual(x) it will return F(x). If you pass a function with two positional arguments, say residual(F, x) then the contract is to modify F inplace.

In < 3.0.0 your code would have automatically assumed you passed a residual(...) function that had a method with two positional arguments such as myfunc(F, x). If it had other methods it would ignore them. Now, we check if myfunc(x::T) where T is the type of the provided x0 exists. If it does, then we construct an inplace function based on that, and call that. If you have both 1 and 2 argument methods, then you need to specify inplace=true manually, nlsolve(f, x; inplace = true).

The problem in your code seems to be https://github.com/JuliaRobotics/IncrementalInference.jl/blob/5065fdfcc0171bf5a8ef977c9bd0df598aa9e138/src/SolverUtilities.jl#L56 which returns nothing, see

julia> typeof(ccw([0.0]))
Nothing

That is why nlsolve gives the error that it does. It takes your 1-arg function and converts it into a new function

f_new(F, x) = copyto!(F, ccw(x))

that then gives you the error that it cannot copy nothing into F.

I need to update the README.md to clarify this this.

TL;DR; If your function (or callable anything) has methods for both one and two arguments, you need to manually say inplace=true from NLsolve >= 3.0.0

julia> nlsolve(ccw, [0.1]; inplace = true)
Results of Nonlinear Solver Algorithm
 * Algorithm: Trust-region with dogleg and autoscaling
 * Starting Point: [0.1]
 * Zero: [8.17033]
 * Inf-norm of residuals: 0.000000
 * Iterations: 7
 * Convergence: true
   * |x - x'| < 0.0e+00: false
   * |f(x)| < 1.0e-08: true
 * Function Calls (f): 8
 * Jacobian Calls (df/dx): 8
dehann commented 5 years ago

Hi @pkofod , thank you very much -- appreciate the help! Think I understand, give me a few days to work through this (just need to sort out some other stuff first).

ASIDE: Currently this package (aka IIF) is using both Optim and NLsolve as we require both root finding and minimization. The single parameter function ccw(x) comes from two different use cases. At some point the plan was to use only NLsolve since my personal preference is to think of algebraic relations in terms of zero-sum equations and then build out everything against zero-crossings. Ultimately IIF is a "fixed-point" method, but for approximating functionals. We are trying to close the gap between nonparametric-MCMC and parametric-gradient-decent.

This issue is probably a good thing, and forces this package to properly refactor a few functions. I tagged IIF v0.4.4 to temporarily limit to NLsolve v2.1.0, but will move to v3.0.0 soon.

I will ping again if I have more questions on this. Thanks again!

cc @GearsAD , @pvazteixeira

dehann commented 5 years ago

HI @pkofod , that was an easy fix -- thanks!

pkofod commented 5 years ago

Cool! Glad it worked out.