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

Fixes and tests on PartialPose3, Bearing, and BearingRange Factors #509

Merged
merged 18 commits into from
Oct 2, 2021

Conversation

dehann
Copy link
Member

@dehann dehann commented Sep 26, 2021

Probably easiest to use Rotations.RotXY or something similar? Issue is the test script was written assuming the coordinates for rotation are Euler angles -- this is probably no longer true, as more general SE(n) objects are now being used.

@dehann
Copy link
Member Author

dehann commented Sep 27, 2021

ah, think the issue with the new factor residual function is that R[1:2,1:2] rotation matrix parts contain more than just the yaw angle. That 2x2 segment also contains information from pitch and roll.

@Affie
Copy link
Member

Affie commented Sep 27, 2021

You might be correct. I just followed the previous factor, but can have another look.

@dehann
Copy link
Member Author

dehann commented Sep 27, 2021

Hi Johan, this is probably still not 100%, but hopefully closer:

wR2 = wR1*1R2 = wR1*(1Rψ*Rθ*Rϕ)
wRz = wR1*1Rz
zRz = wRz \ wR(Δψ)

M_R = SO(3)
δ(α,β,γ) = vee(M_R, R_0, log(M_R, R_0, zRz))

M = SE(3)
p0 = identity_element(M)
δ(x,y,z,α,β,γ) = vee(M, p0, log(M, p0, zRz))

EDIT, best to run the residual in SE(2), however, note that all three Lie exp rotation coordinates will be affected, so the partial needs to involve for XYYaw = (1,2,4,5,6)

EDIT2: the main issue is which reference frame should XYYaw work in? The assumption is this XYYaw<:AbstractRelative will be used in combination with ZRP priors. The combo of two factors replace a regular Pose3Pose3 odo. Therefore, XYYaw is assumed (from the users perspective) as a factor being in the "world frame", but as a wander-azimuth relative to wT1. So by extension, XY should be considered as a local level XY also relative to wander-azimuth wT1. Again, users needs to know that this XYYaw factor is used in combination with a Pitch-Roll-Z prior (which creates the assumed local level).

@Affie
Copy link
Member

Affie commented Sep 29, 2021

Hi @dehann. As far as I can tell, I missed the projection part to make sure the SE(2) points are on the manifold.
We should also maybe assert that the pitch is not 90 degrees?

Comment on lines 332 to 346
##

# ensure the newly updated values match what is specified in mu2
@show mu2 # mu2 is used for XYYaw
# trying to compare world and body frame values -- not right!
@show Statistics.mean(pts[newdims,:],dims=2)
@test_broken sum(abs.(Statistics.mean(pts[newdims,:],dims=2)-mu2) .< [1.5;1.5;0.3]) == 3

# ensure a re-evaluation of the partial factor updates the partial variable dimensions correclty
@test norm(X2pts[newdims,:] - pts[newdims,:]) < 1.0

# ensure that memory pointers are working correctly
memcheck = getVal(v2)
# @test norm(X2pts - memcheck) < 1e-10

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These looks outdated as pts no longer exists

@Affie Affie added the test all the things Test to improve coverage label Oct 2, 2021
@Affie
Copy link
Member

Affie commented Oct 2, 2021

Should have probably done BearingRange fix in a new branch, but it just reverts back to when it last worked. It should be correct now and we can update and make sure it still passes the tests.

@Affie Affie changed the title wip on testPartialPose3 failure wip on testPartialPose3 Bearing and BearingRange failure Oct 2, 2021
@Affie Affie changed the title wip on testPartialPose3 Bearing and BearingRange failure Fixes and tests on PartialPose3 Bearing and BearingRange Factors Oct 2, 2021
@Affie Affie changed the title Fixes and tests on PartialPose3 Bearing and BearingRange Factors Fixes and tests on PartialPose3, Bearing, and BearingRange Factors Oct 2, 2021
@Affie
Copy link
Member

Affie commented Oct 2, 2021

Potential bug remains in partials

@Affie
Copy link
Member

Affie commented Oct 2, 2021

I'm happy to merge this and do further improvements in new PRs. CI failures are intermittent numerical failures.

@Affie Affie added the upstream label Oct 2, 2021
@Affie Affie marked this pull request as ready for review October 2, 2021 10:08
@Affie Affie merged commit 800270d into master Oct 2, 2021
@dehann
Copy link
Member Author

dehann commented Oct 2, 2021

cool thanks! I was looking at bearing tests yesterday and trying to see if there is a bug or shortcoming in IIF, but wasn't yet able to pin down either way yet. Think we're close to tagging IIF.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants