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

[RST-1926] extend local param definition #40

Merged
merged 10 commits into from Apr 24, 2019

Conversation

svwilliams
Copy link
Contributor

@svwilliams svwilliams commented Apr 8, 2019

This PR extends the definition of a LocalParameterization to include a Minus() function, the conceptual inverse of the Plus() function. This is a prerequisite for marginalization support.

The Orientation2D and Orientation3D variable classes will be temporarily broken. An upcoming PR (#41) will fix those classes up separately. That means some of the orientation/pose unit tests will be broken.

Scope Creep:
cc91396 - I was cleaning up the Orientation2D class and ended up breaking the Path2D publisher. It relied on non-standard functionality of the variable classes. So I ended up fixing the Path2D publisher in the PR as well.

@svwilliams svwilliams requested a review from ayrton04 April 8, 2019 19:32
*
* And the second functor should compute the inverse operation:
*
* Minus(x1, x2) -> delta, such that Minus(x, Plus(x, delta)) = delta
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be clearer to say "such that Plus(x1, delta) = x2"?

*
* For more information on local parameterizations, see fuse_core::LocalParameterization
*/
template <typename PlusFunctor, typename MinusFunctor, int kGlobalSize, int kLocalSize>
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the variable naming scheme is for consistency with Ceres.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going to drop the leading k, but ceres::LocalParameterization defines GlobalSize() and LocalSize() methods. That means the template names cannot also be GlobalSize and LocalSize. Since I didn't have any better ideas, I went with the kGlobalSize and kLocalSize from the Ceres implementation. Happy to change it if you have a better suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah, that's fine by me.

double* jacobian) const
{
double zero_delta[kLocalSize];
for (int i = 0; i < kLocalSize; ++i)
Copy link
Contributor

Choose a reason for hiding this comment

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

Doubt it's any faster, but can you use std::fill here?

std::fill(zero_delta, zero_delta + kLocalSize, 0.0);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are actually zero-initialized by the compiler. Removing the initialization entirely.
https://en.cppreference.com/w/cpp/language/zero_initialization

double f[3]; // zero-initialized to three 0.0's

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, sweet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, maybe I'm lying. That's only for static/global-scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But empty brace-initialization does work.
double zero_delta[kLocalSize] = {};

Copy link
Contributor

Choose a reason for hiding this comment

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

Yay C++14.

@svwilliams svwilliams force-pushed the RST-1926-extend-local-param-definition branch 2 times, most recently from 9edbe82 to d38513a Compare April 17, 2019 18:16
@svwilliams svwilliams force-pushed the RST-1926-extend-local-param-definition branch from d38513a to 7b0db62 Compare April 17, 2019 18:17
@svwilliams svwilliams force-pushed the RST-1926-extend-local-param-definition branch from e56fa3f to fd8604b Compare April 24, 2019 16:24
@svwilliams svwilliams merged commit 60c4814 into devel Apr 24, 2019
@svwilliams svwilliams deleted the RST-1926-extend-local-param-definition branch April 27, 2019 03:20
svwilliams added a commit that referenced this pull request Jun 19, 2019
* [RST-1926] Extend the local parameter definition to include Minus() (#40)
* [RST-1927] Update the local parameterization for the orientation variables (#41)
* [RST-1940] Added a localSize() method to the Variable class (#42)
* Fixed missing header. It was moved to a different package. (#49)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants