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 Bundle and BundleTangent types #234

Merged
merged 11 commits into from
Jul 21, 2021
Merged

Add Bundle and BundleTangent types #234

merged 11 commits into from
Jul 21, 2021

Conversation

artivis
Copy link
Owner

@artivis artivis commented Jul 10, 2021

Replaces #175.
See #175 comments for a discussion around the feature.

Closes #84.
Closes #175.

Notes

  • Ubuntu 16.04 is deprecated in CI. The distro is EOL and it's support will be soon dropped in github actions (that's fortunate because this PR causes a compiler internal error on 16.04)
  • Bundle is disabled on MSVC-based CI. Looks like MSVC is full of issues with alias-type and it doesn't like the Rn alias (R1/R2/R3 etc...)

Original PR description

I felt compelled to take a stab at #84 and this is the result.

I believe it is functional as it is, though it could for sure use more testing. Wanted to see what you think before proceeding.

Some comments:

  • Both Composite and CompositeTangent are templated on the underlying lie group type, i.e. CompositeTangent<double, SE3, R2> (and not CompositeTangent<double, SE3Tangent, R2Tangent>) is the tangent of Composite<double, SE3, R2>.
  • Both types have a member function template<size_t I> get() that allows access to the individual parts via a Map. I would have preferred to inherit from std::tuple to get std::get for free but it seems like it is assumed throughout the library that all types store the underlying data as a single continuous array.
  • I first wrote it for C++17 and had to work quite hard to convert it to C++11. As a result there is a replacement for C++14 std::integer_sequence and the previous C++17 fold expressions are now using this C++11 initializer_list trick.
  • Some library parts are a bit unnatural for the composite type, such as the LieAlg and Transformation types. They are there so that the full interface is implemented and the tests run
  • Jacobian and Transformation are block-diagonal and could potentially be represented with sparse instead of dense matrices.

@artivis artivis added the enhancement New feature or request label Jul 10, 2021
@artivis artivis self-assigned this Jul 10, 2021
@artivis artivis marked this pull request as draft July 10, 2021 21:07
@artivis artivis force-pushed the pettni/devel branch 2 times, most recently from 78ad463 to 9316d81 Compare July 11, 2021 16:54
@codecov
Copy link

codecov bot commented Jul 11, 2021

Codecov Report

Merging #234 (361822d) into devel (436702d) will increase coverage by 0.45%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            devel     #234      +/-   ##
==========================================
+ Coverage   97.71%   98.17%   +0.45%     
==========================================
  Files          49       55       +6     
  Lines        1532     1753     +221     
==========================================
+ Hits         1497     1721     +224     
+ Misses         35       32       -3     

@artivis artivis force-pushed the pettni/devel branch 2 times, most recently from 84d7c53 to e6922b2 Compare July 11, 2021 19:15
@artivis artivis marked this pull request as ready for review July 11, 2021 19:56
@artivis
Copy link
Owner Author

artivis commented Jul 11, 2021

@pettni I opened this PR as a replacement to yours #175. I initially planned on opening a PR against your fork but there are conflicts due to this branch being rebased on devel. I thus went for the lazier simpler solution.
If you're fine with that and this PR content, I'll merge it anytime soon. The main changes since last time I pinged you are listed in the PR description.

@artivis
Copy link
Owner Author

artivis commented Jul 21, 2021

While I didn't get any answer, I'm going to proceed merging this PR and closes #175.

@artivis artivis merged commit 317ff22 into devel Jul 21, 2021
@artivis artivis deleted the pettni/devel branch August 30, 2021 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: support for composite groups
2 participants