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

New localProduct too eager on partials as 1DOF only #1206

Closed
dehann opened this issue Mar 16, 2021 · 3 comments · Fixed by #1220 or #1221
Closed

New localProduct too eager on partials as 1DOF only #1206

dehann opened this issue Mar 16, 2021 · 3 comments · Fixed by #1220 or #1221

Comments

@dehann
Copy link
Member

dehann commented Mar 16, 2021

partials means dimension constrained, as in only some dimensions are effected by the partial factor. Something like a Range factor operates on XY (Z) dimensions simultaneously through the Minimization process. It is wrong to overconstrict partials to work one dimension at a time. Related issue #1010 .

MWE:

using RoME

fg = initfg()
getSolverParams(fg).useMsgLikelihoods = true
addVariable!(fg, :x0, Pose2)
addVariable!(fg, :l1, Point2)
addFactor!(fg, [:x0], PriorPose2( MvNormal([0; 0; 0], diagm([0.3;0.3;0.3].^2)) ))
ppr = Pose2Point2Range(MvNormal([7.323699045815659], diagm([0.3].^2)))
addFactor!(fg, [:x0; :l1], ppr )
addVariable!(fg, :x1, Pose2)
pp = Pose2Pose2(MvNormal([9.82039106655709;-0.040893643507463134;0.7851856362659602], diagm([0.3;0.3;0.05].^2)))
ppr = Pose2Point2Range(MvNormal([6.783355788017825], diagm([0.3].^2)))
addFactor!(fg, [Symbol("x$(i-1)"); Symbol("x$(i)")], pp )
addFactor!(fg, [Symbol("x$(i)"); :l1], ppr )


ensureAllInitialized!(fg)
pp, parr, partials, lb = IIF.localProduct(fg, :l1)

@assert length(partials) == 0

NOTE, RoME.Pose2Point2Range is indicated as a partial

https://github.com/JuliaRobotics/RoME.jl/blob/03129f5bd100a3d6e2600bf692b98c9bb9e928c4/src/factors/Range2D.jl#L51

@dehann dehann added this to the v0.21.5 milestone Mar 16, 2021
@dehann dehann changed the title New localProduct too eager on partials New localProduct too eager on partials as 1D only Mar 16, 2021
@dehann dehann changed the title New localProduct too eager on partials as 1D only New localProduct too eager on partials as 1DOF only Mar 16, 2021
@dehann
Copy link
Member Author

dehann commented Mar 17, 2021

Need to update (adding to ongoing AMP.MKD upgrades):

# do each partial dimension individually
for (dimnum,pp) in partials
pGM[dimnum,:] = AMP.manifoldProduct(pp, (manis[dimnum],), Niter=1) |> getPoints
end

# do each partial dimension individually
for (dimnum,pp) in partials
pGM[dimnum,:] = AMP.manifoldProduct(pp, (manis[dimnum],), Niter=1) |> getPoints
end

@dehann
Copy link
Member Author

dehann commented Mar 18, 2021

The plan is to do Manifolds.jl consolidation first

as the last step in this epic.

cc @Affie

@dehann
Copy link
Member Author

dehann commented Mar 26, 2021

Hi @lemauee,

Just an update. I think this issue is a big part of the problems you were seeing in JuliaRobotics/RoME.jl#378, JuliaRobotics/RoME.jl#380 where Pose2Point2Range factors were losing correlation between dimensions because partials we treated too naively (1DOF at a time).

A workaround is easy, but Optim.jl has to work harder. The proper fix is to combine with Manifolds.jl first (what i have been doing the past 3 weeks) and rebuild the approx belief product wrappers.

Basically, we have incompatible partials and Optim.jl plumbing in the existing/old code. This was not an issue before, but in consolidating parametric and nonparametric plumbing this incompatibility showed up through the refactoring. Previously Optim was just grinding away until the right answer was found.

Building out the new abstractions with Manifolds.jl is the right place to make these two aspects compatible. The fix should be available either in IIF v0.22.x or latest v0.23.0 (depending on how semver deprecation rules play out).

Best,
Dehann

dehann added a commit to JuliaRobotics/RoME.jl that referenced this issue Apr 11, 2021
@dehann dehann modified the milestones: v0.23.x, v0.23.1 Apr 11, 2021
This was linked to pull requests Apr 11, 2021
@dehann dehann self-assigned this Apr 11, 2021
@dehann dehann closed this as completed Apr 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment