-
Notifications
You must be signed in to change notification settings - Fork 129
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
PGO: Rotation/translation swapped in relative pose noise #482
Conversation
Thanks for the fix. Can you also swap the values in the |
Ah, I missed that - thanks for the pointer! Turns out they will still fail because the noise model for the pose graph has translation and rotation in the correct order. Should we re-run the experiment with the correct ordering, and modify the |
Yes, in this case I think is safe to modify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the fix! Feel free to merge once you push the update to expected losses and CI passes.
@suddhu Any plans on finalizing this PR soon? Thanks! |
Yup - sorry for taking a bit! Running With the corrected rot/trans noise (latest commit on this branch): pgo_cpu_choleskydensesolver_64_256_16.mat
[[-0.298862789376761, -0.30542158053746615, -0.27485602387525104, -0.300523110327785]]
pgo_cuda_lucudasparsesolver_64_256_16.mat
[[-0.2988627893768147, -0.3054215805374096, -0.27485602387600594, -0.3005231103295129]]
pgo_cpu_cholmodsparsesolver_64_256_16.mat
[[-0.29886278947606854, -0.3054215806396245, -0.2748560240408431, -0.30052311060619713]] Previous result (main branch): pgo_cpu_choleskydensesolver_64_256_16.mat
[[-0.05253952558117652, -0.06922697773243278, -0.036454724771805606, -0.06110373107271005]]
.pgo_cuda_lucudasparsesolver_64_256_16.mat
[[-0.05253952558136447, -0.06922697773270092, -0.03645472477181133, -0.06110373110087176]]
.pgo_cpu_cholmodsparsesolver_64_256_16.mat
[[-0.052539527012601166, -0.06922697775065781, -0.03645472565461781, -0.06110373151314644]] Would that be expected, or anything I overlooked @fantaosha? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the noise ratios for rotation and translation are switched.
See my last comments. Thanks! |
@suddhu This is not directly related to this PR, but could you also add the change below in |
@fantaosha swapped the noise ratios and the rotation/translation in When I swap the ordering of translation, rotation in the noise model to |
Overridden by #560 |
Motivation and Context
Rotation and translation noise are swapped in the PGO noise addition for
pose_graph/dataset.py
(Issue #481). The change merely fixes this to give a se(3) noise tangent vector [noise_translation, noise_rotation]How Has This Been Tested
Minor bug fix, non-breaking change
Types of changes
Checklist