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

Add a MagPoseFactor for using magnetometer measurements with Pose2/Pose3 #752

Merged
merged 4 commits into from
Jun 3, 2021

Conversation

miloknowles
Copy link
Contributor

This PR adds a new MagPoseFactor<POSE> which extends the existing MagFactor to work with Pose2 and Pose3. The factor also includes an optional body_P_sensor in case the magnetometer does not coincide with the body frame. I implemented this factor so that I could use magnetometer measurements within a VIO system, and thought it might be useful to others. It doesn't seem like the MagFactor gets much use, so no worries if you don't think this factor should be added.

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

Needs some comment and minor syntax updates.

gtsam_unstable/slam/MagPoseFactor.h Outdated Show resolved Hide resolved
gtsam_unstable/slam/MagPoseFactor.h Outdated Show resolved Hide resolved
gtsam_unstable/slam/MagPoseFactor.h Outdated Show resolved Hide resolved
gtsam_unstable/slam/MagPoseFactor.h Outdated Show resolved Hide resolved
gtsam_unstable/slam/MagPoseFactor.h Outdated Show resolved Hide resolved
gtsam_unstable/slam/MagPoseFactor.h Outdated Show resolved Hide resolved
gtsam_unstable/slam/MagPoseFactor.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

LGTM

@varunagrawal
Copy link
Collaborator

Sorry for the ridiculous amounts of reviews. Github was unresponsive for a while and I have no idea what happened.

@varunagrawal
Copy link
Collaborator

@dellaert should we consider moving both MagFactor and MagPoseFactor from gtsam_unstable to gtsam?

@miloknowles
Copy link
Contributor Author

Thanks for the review, @varunagrawal! Made the changes you requested and will wait on the decision about moving out of gtsam_unstable.

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

LGTM

@varunagrawal
Copy link
Collaborator

@miloknowles we can always consider moving it out of gtsam_unstable in another PR. Let's merge this for now.

@varunagrawal varunagrawal merged commit 5540097 into borglab:develop Jun 3, 2021
@tmcg0
Copy link
Contributor

tmcg0 commented Jun 3, 2021

Sorry to comment on a closed PR, but glad this got merged! I had actually built a custom factor to do this in my own work and this factor is way more robust, tested, and flexible than the one I had made, so thanks Milo!

I just wanted to throw in my 2 cents that this would be great to move out of gtsam_unstable. I'm not sure what determines what goes in unstable vs. not, but I can't think of any major deficiencies with this w.r.t. manifold discontinuities, numerical conditioning, or instability of any subcomponent.

@dellaert
Copy link
Member

dellaert commented Jun 3, 2021

Awesome. Fork, move, PR :-)

@miloknowles
Copy link
Contributor Author

@tmcg0 feel free to open the gtsam_unstable --> gtsam PR if you're already working on it! Otherwise I'm happy to take care of that.

@tmcg0
Copy link
Contributor

tmcg0 commented Jun 3, 2021

@tmcg0 feel free to open the gtsam_unstable --> gtsam PR if you're already working on it! Otherwise I'm happy to take care of that.

You can go ahead and take care of it! I haven't started it and I'm sure I'd mess up something with the build scripts or python bindings and have to spend a lot of time backtracking fixes.

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

Successfully merging this pull request may close these issues.

4 participants