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

Use Manifold instead of LocalParameterization. #1882

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zjwoody
Copy link
Contributor

@zjwoody zjwoody commented May 12, 2022

We need to migrate the Cartographer code to use Manifolds instead of LocalParameterization to allow Ceres code to remove the LocalParameterization code.

The PR converts internal use of LocalParameterization to Manifold. Removes headers that reference local_parameterization or implementation classes.

Note that internal variable and method names with type Manifold were renamed Manifold/manifold, but the outer abstraction class, proto name and so on were left as Parameterization/parameterization.

Signed-off-by: Jie Zheng zhengj@google.com

@wohe
Copy link
Member

wohe commented Jul 28, 2022

@zjwoody Sorry, it took slightly longer than expected. Now CI should work again. Can you rebase your PR? I also had a quick look at it already, and my understanding is that it will not compile as is:

  1. We support Ubuntu and Debian releases until LTS ends, so for several years we will have to be able to use a Ceres version which does not yet support Manifold.
  2. Thus, we would need to guard using a version number as suggested in ceres::LocalParameterization has been deprecated in the Ceres Solver 2.1.0, migrate to using ceres::Manifold #1879.
  3. Just FYI, this should not delay this PR in any way. If I understand correctly then both Ubuntu 22.04 and Debian Bullseye do not support Manifold, so CI would not be able to test the Manifold related code. But it seems the following releases when they happen will pick it up. Probably 2024 if Ubuntu releases as usual, and whenever Debian does.

Signed-off-by: zjwoody <zhengj@google.com>
@zjwoody
Copy link
Contributor Author

zjwoody commented Aug 2, 2022

Hi Wolfgang, thanks for the information!
I rebased the PR and it is not able to compile due to the reason you mentioned, so it can't be merged.
I have some stupid questions since I'm not clear about what to do next:

  1. For the guard of ceres version number, do you mean limit the ceres version used in Cartographer to the old version(e.g. < 2.1.0) before it switched to the manifold?
  2. If 1 is true and we guard the ceres version, what should we do with this pr?
    Thanks!

@wohe
Copy link
Member

wohe commented Sep 5, 2022

Sorry about the slow response. I was referring to the suggestion from #1879. Basically, you have to put two versions of the code in some places like this, checking whether a recent Ceres version is used:

#if CERES_VERSION_MAJOR > 2 || CERES_VERSION_MAJOR == 2 && CERES_VERSION_MINOR >= 1
// TODO: new code here Manifold
#else
// TODO: old code here LocalParameterization
#endif

You might have to #include "ceres/version.h". Does that make sense?

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.

None yet

2 participants