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

Fixing broken MATLAB wrapper #4

Open
2 tasks
tmcg0 opened this issue Nov 13, 2020 · 2 comments
Open
2 tasks

Fixing broken MATLAB wrapper #4

tmcg0 opened this issue Nov 13, 2020 · 2 comments
Labels
bug Something isn't working compatibility features which increase compatibility

Comments

@tmcg0
Copy link
Owner

tmcg0 commented Nov 13, 2020

Recently, bioslam was upgraded to be compatible with exactly GTSAM 4.0.3 (as opposed to my previous hacked-up version of GTSAM). This upgrade broke the MATLAB wrapper. Issues affecting fixing the MATLAB wrapper should be documented here.

  • Update MATLAB code to handle preintegrated (combined) IMU measurements and parameters.

  • this shouldn't be too hard. In GTSAM 4.0.3, it looks like the wrapper code has been updated to expose these interfaces, which is why I previously was hacking up a version of GTSAM to do exactly that. Go through and find MATLAB examples in the GTSAM repo to update your code.

  • write documentation to show users how to edit GTSAM according to my PR which gave a little more functionality that we need in the bioslam MATLAB implementation.

@tmcg0 tmcg0 transferred this issue from another repository Dec 15, 2020
@tmcg0 tmcg0 added bug Something isn't working compatibility features which increase compatibility labels Jan 26, 2021
@tmcg0
Copy link
Owner Author

tmcg0 commented Apr 29, 2021

Quick workaround added in this commit: in MATLAB when you try to construct an ImuPoseEstimator with dynamic bias, it will default back to using static bias. This works around the bug for now, but things will still break if you think your ImuPoseEstimator was setup with dynamic bias estimation. This is only a temporary fix but will work for many applications.

(This workaround works because the problem is only with gtsam.CombinedPreintegrationParams, which doesn't affect the normal ImuFactor)

@tmcg0
Copy link
Owner Author

tmcg0 commented May 12, 2021

Note for clarification/reminder: the bioslam MATLAB wrapper request gtsam.PriorFactorUnit3, which was only introduced after my aforementioned PR to GTSAM on Nov 4, 2020. That means no current GTSAM release will have this fix (but the soon-to-be-released GTSAM 4.1 will). This means that I have to have users manually edit their GTSAM for GTSAM<4.1, because the edits I made to GTSAM weren't just a new prior factor on unit3, but also expanded the GTSAM Values functionality with insert/update/at function for Unit3 types. This is messy and I hate it, but bioslam is going to have to be for GTSAM 4.1+ or else modifications to GTSAM will be necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compatibility features which increase compatibility
Projects
None yet
Development

No branches or pull requests

1 participant