Consolidate Range Factors

[Affie]

Difficult transition that required IIF to add inflation (JuliaRobotics/IncrementalInference.jl#1051), but is expected to result in a smooth transition out of the major consolidation effort (JuliaRobotics/IncrementalInference.jl#467). For parity, the minimum requirement for is now IIF v0.21.2.

[dehann]

Ah wait, .inflation is not working as well as it should yet. This next case should clearly produce a donut density but it's still scattered:

[Affie]

It looks like inflation is doing what it should:

0.0

0.1

1.0

10.0

[Affie]

Another interesting case to test with is:

# I think this should be added to RoME permanently?
RoME.PriorPoint2(::UniformScaling) = PriorPoint2(MvNormal(ones(2)))

fg = initfg()
fg.solverParams.inflation = 10.

addVariable!(fg, :x1, Point2)
addVariable!(fg, :l1, Point2)

# prior = PriorPoint2(MvNormal([1.,1], [0.01, 0.01]))
prior = Mixture(PriorPoint2, (MvNormal([1.,1], [0.01, 0.01]), MvNormal([10.,10], [0.01, 0.01])), (0.4,0.6))
addFactor!(fg, [:l1], prior)

addFactor!(fg, [:x1; :l1], Point2Point2Range(Normal(5, 0.01)))

solveGraph!(fg)
pl = plotKDE(fg, ls(fg))
Gadfly.draw(PDF("file.pdf",20cm,20cm),pl)

[Affie]

The Euclidian distance factors definitely work in both directions. xref #186

[Affie]

Hi @dehann, exploring ways to test doughnut distributions I came across HypothesisTests and tried this:

using HypothesisTests
##
N = 300 
C = [1;1] # donut centre
# P = rand(getKDE(fg, :x2), N) .- C
P = getVal(fg, :x2) .- C
Θ = atan.(P[2,:], P[1,:])
t = ExactOneSampleKSTest(Θ, Uniform(-pi,pi))
@info "Θ Uniform" pvalue(t)
# StatsPlots.histogram(Θ,bins=10)
StatsPlots.histogram(Θ,bins=range(-pi-pi/8, stop = pi+pi/8, length = 21))
##
R = sqrt.(P[1,:].^2 .+ P[2,:].^2)
t = ExactOneSampleKSTest(R, Normal(1.0,0.01))
@info "R Normal" pvalue(t)

StatsPlots.histogram(R,bins=20)

PS. rand(getKDE(fg, :x2), N) didn't seem to work that well...

[dehann]

EDIT, this was quickly disproven -- issue was due to difficulty in testing these features in isolation. IIF v0.21.2 added significant more tests to check that this is in factor working properly.

---

Ah, there might a weird problem where the numerical solving is over eager towards solutions that are near (0,0). I don't understand it yet or fully convinced that it's happening, but just want to make a note. This is in prep for IIF v0.21.2.

The experiment I'm doing is a donut range belief ring around (0,100) at range 100. Therefore a ring through (0,200) as well as (0,0). Qualitatively, it seems probable that even though solutions are started near (0,200) (with inflation noise added after) that solutions near (0,0) are favored. The same thing happens with inflation noise around solutions that are started near (0,0).

[Affie]

fully convinced that it's happening

Haha, I thought the same thing and then I convinced myself it wasn't happening. So maybe there is something to it...

What I looked at was the x1=(0,100) x2=(100,0) range=100 example. It looked to favour (0,0), but then I thought it was initialization and the solver not spreading enough with one Gibbs iteration.

[Affie]

@dehann, I had a look at this again and it seems like this line has no effect in the MVE (only x1 and l1):

dummy, d = fmcmc!(dfg, cliq, inmsgs, cliqdata.directFrtlMsgIDs, N, 2, dbg, logger, true)

cliqdata.directFrtlMsgIDs = []

If I force cliqdata.directFrtlMsgIDs = [:l1] it starts having an effect.

With the 3 variable example, it's still broken.

I'll look more into it

[dehann]

yeah, dont worry about directFrtlMsgID now, the main issue is about test cases to see if Optim.jl has a bias towards 0 (just using approxConv and initManual!)?

[dehann]

Hi @lemauee, just keeping you in the loop. We found a possible weird thing happening (perhaps as high upstream as Optim.jl). Current IIF#master and RoME#master should already go a long way to fixing your issue RoME #380. Johan is busy building a new set of tests in IIF to make sure this potential underlying issue is (or is not) a thing. We will tag RoME v0.13.0 soon thereafter. We know you have to compute final results very soon.

The potentially weird issue looks like in Optim.jl (or maybe in IIF) ... as though there is a preference for numerical values close to 0,0. Sounds crazy, but its pretty hard to see in regular use. We think we have a workable testcase to isolate the problem and this is what Johan is coding up now. In short, if we evaluate the conditional belief through a range measurement, we should get a "donut" ring proposal density. This works fine if the ring distribution is away from 0,0. Everything looks nice and acts as we expect. However, as soon as the ring distribution intersects or is near 0,0, the resulting density concentrates around 0,0 (i.e. "does not inflate all the way as a circular density"). The new tests should clearly demonstrate or negate this potential issue, and then we can fix / tag IIF v0.21.2 from there. This PR in RoME will be best done after that issue is isolated (so we can keep good unit tests after the big consolidation of IIF 467) -- i.e. this issue is holding up the show for RoME v0.13.0, and is our main priority to fix quickly.

[lemauee]

Hi @dehann, thanks for keeping me in the loop. Let me know if there's anything I can contribute :)

[Affie]

@dehann, I have a few observations about major effects in this:

Initialization

• Probably the biggest effect if you can initialize with at least some belief at the correct place it makes a big improvement.
• So should we perhaps start off with the values from the source and not zero. (If that is what is done...I'm not 100% up to speed how this is currently done, will have to dig more)
  Figures are Optim input vs output for x1,l1 mixed
  Bad
  
  Good
  

Optimization

• Should the spread not happen within the optimization (probably based on the gradient rather than the values).
• Something like Optim.ParticleSwarm() or perhaps we should try automatic differentiation?

[dehann]

All right, I have some good news. This was wrong (luckily nothing wrong with Optim.jl that I can find at this stage):
#416 (comment)

And, this was the wrong direction to go in (our initial test):
#416 (comment)

Instead, we decided to go in this direction:
JuliaRobotics/IncrementalInference.jl@5b88b2c#diff-8972b09686be996c3539aa2f2e10939b8c48c0a01efe046c6bd5c2aaeb10c9c5R285-R289

Basically, to make sure the proposal inflation works to cover the changes from IIF 467 and this RoME 416, the easiest way is to introduce a new params.inflationCycles=3 (as default). That means convolutions are triple computed, and each time the target points are consecutively inflated and resolved. This does not yet get around very large disconnected modes native within a single factor which can happen in 1D range examples (FYI that modes from usual multihypo, Mixtures, and >1D modes are still fairly well explored as before even if well separated).

So until IIF 1010, AMP 41 and RoME 244 are done, Optim and NLsolve will be called more often -- luckily that code is somewhat efficient. It is important that we keep quality high at this time rather than try shave for performance too early. We will be going into a code performance round around April/May if things progress sufficiently here.

Should the spread not happen within the optimization (probably based on the gradient rather than the values

Jip, think that is pretty much what happens now in IIF v0.21.2 -- hope I'm understanding you right here.

Back to the issue, I believe that these sets of fixes in IIF and RoME not only concludes a 16 month epic to consolidate nonparametric and parametric factor definitions in a clean and autodiff friendly way, but also showed new issues first discovered by Leo as we were transiting through to this post IIF 467 world. I believe the fixes in RoME v0.13.0 will be good enough to close out #380. #378 might also be solved, but that is a separate effort I will get into now.

To keep folks informed about my plans, IIF 1010 and AMP 41 / RoME 244 are high priority issues for me.

[dehann]

Something like Optim.ParticleSwarm()

Think the triple inflation rounds kind of get at the same thing, so lets maybe skip this for now and move to IIF 1010 instead.

or perhaps we should try automatic differentiation?

Sure, if it is easy to try and falls back for strange factors. I'm not sure best to interact with user on when autodiff is on or off, but I'm wary of creating too many fiddle options and constant breakages. So I would say autodiff should be OFF by default for now, until we have the time and space to design the user experience properly?

[dehann]

Hi @lemauee ,

In addition to the new comments above (#416 (comment)) ... sure we happy to keep you informed for processing results, up to date info is important for a March deadline.

Let me know if there's anything I can contribute :)

• If you could try RoME v0.13.0 with the two new "fixes" that were required post IIF 467. Let us know how it goes. Going to cc @GearsAD on this too.
• Leo, you mentioned you compiled more code via PackageCompiler.jl for RoME and Plotting. Would you perhaps mind sharing that compile script with the community please? Either via PR to the existing RoME compile directory or just via an issue with code dump would work too ... we can incorporate that for more speedy startup time. (xref Loading a RoMEPlotting sysimage fails RoMEPlotting.jl#140)

Apologies these fixes took longer than expected but hopefully #380 will run a lot better now.

Best,
Dehann

EDIT, please check through recent comments properly. I had multiple questions and responses...

[codecov-io]

Codecov Report

Merging #416 (792c389) into master (bae9dbb) will increase coverage by 3.42%.
The diff coverage is 42.85%.

@@            Coverage Diff             @@
##           master     #416      +/-   ##
==========================================
+ Coverage   21.19%   24.61%   +3.42%     
==========================================
  Files          44       43       -1     
  Lines        1779     1678     -101     
==========================================
+ Hits          377      413      +36     
+ Misses       1402     1265     -137     

Impacted Files	Coverage Δ
src/factors/Range2D.jl	22.22% <42.85%> (-1.12%)	⬇️
src/factors/flux/FluxModelsPose2Pose2.jl
src/factors/Pose2D.jl	88.23% <0.00%> (+5.88%)	⬆️
src/CanonicalGraphs.jl	45.23% <0.00%> (+7.14%)	⬆️
src/factors/flux/MixtureFluxPose2Pose2.jl	50.00% <0.00%> (+50.00%)	⬆️
src/factors/flux/models/Pose2OdoNN_01.jl	85.71% <0.00%> (+85.71%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update faae3cb...792c389. Read the comment docs.

[dehann]

Oh, @lemauee one other thing. In your case, before IIF 1010 is done. You can also try to use treeinit (an experimental feature we have been maturing for many months). I found with this testing that sometimes the treeinit works better than the graphinit.

fg = initfg()
getSolverParams(fg).treeinit = true # might already be obsolete
getSolverParams(fg).graphinit = false

# then to build factors without graph init
addFactor!(fg, [...], ..., graphinit=false)

# note fg will not have numerical values yet

# solving the Bayes tree will then also calculate the initial values for the graph based on the tree structure
solveTree!(fg);

# check results
getBelief(fg, :x17) |> getPoints

# or post processed point values from the estimated marginal beliefs
getPPE(fg, :x17).suggested

While we are here, you can tweak .inflation per factor if there is good reason to do so:

# to override global (`getSolverParams(fg).inflation=5`) on a per factor basis, do:
addFactor!(...., inflation=3) # value 3-5 are nominal with good empirical performance, larger is more aggressive, 0 is off.

[lemauee]

Thanks for the thorough information and all the work put into this, I'll get right to testing now and will report back soon!

[lemauee]

See #380 for the results, unimodal with small noise looks fine :)