Select mixin components at runtime #126

nahueespinosa · 2023-03-03T03:03:24Z

Closes #118.

Summary

This solves the runtime component selection problem using static dispatch and variants. There were several yaks I had to shave in the process:

Cleanup external interface using type erased views.
Remove the likelihood field publisher.
Rename internal mixin methods.
Separate storage functionality in its own mixin type.
Re-add an estimation mixin.
Update ciabatta to simplify mixin definition.

Checklist

Read the contributing guidelines.
Configured pre-commit and ran colcon test locally.
Signed all commits for DCO.
Added tests (regression tests for bugs, coverage of new code for features).
Updated documentation (as needed).
Checked that CI is passing.

nahueespinosa · 2023-03-03T03:09:37Z

@ivanpauno @glpuga @hidmic If you have time to review, let me know what you think.

Documentation and testing is left for after the rest is approved.
The idea would be to merge everything that's in the works and rebase this on top of the latest version.
It would also be interesting to profile this and compare with current main.

ivanpauno

Cool work!!

I have left some minor comments/questions/suggestions.
Tests and docstrings need to be updated, when that's ready I think this is looking good!

beluga/include/beluga/algorithm/estimation.hpp

beluga/include/beluga/algorithm/sampling.hpp

beluga/include/beluga/algorithm/particle_filter.hpp

beluga/include/beluga/motion/differential_drive_model.hpp

beluga/include/beluga/localization.hpp

beluga_amcl/src/amcl_node.cpp

ivanpauno

Once tests are readded, documentation is completed this looks ready to go to me.
This is looking great!

ivanpauno · 2023-03-03T16:42:20Z

It would also be interesting to profile this and compare with current main.

+1, I think we should do that before merging.
I can help with that if you want, as I already have the results for main from my computer.

hidmic

Left some comments. Just pestering a bit. Great work @nahueespinosa.

beluga/include/beluga/localization.hpp

beluga/include/beluga/sensor/likelihood_field_model.hpp

beluga_amcl/src/amcl_node.cpp

beluga/include/beluga/localization.hpp

beluga/include/beluga/mixin.hpp

beluga_amcl/src/amcl_node.cpp

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

hidmic

I'll review tests tomorrow (not enough brain right now). Super clean code @nahueespinosa 🚀

beluga/include/beluga/motion/omnidirectional_drive_model.hpp

beluga_amcl/src/amcl_node.cpp

beluga/include/beluga/localization.hpp

beluga/include/beluga/mixin/descriptor.hpp

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

`gcc` and `clang` compile them, but are technically not valid C++. Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

beluga_example/config/params.yaml

Also, take an rvalue reference. Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

glpuga

LGTM! Great work!

ivanpauno · 2023-03-08T17:54:45Z

Performance comparison:

Red -> this PR
Blue -> last recording

Which looks like nothing significant changed 😃 🎉

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

nahueespinosa · 2023-03-13T12:01:18Z

@ivanpauno @hidmic @glpuga Documentation and tests were updated! PTAL when you can.

ivanpauno

I have some minor comments, LGTM!

beluga/include/beluga/algorithm/particle_filter.hpp

beluga/include/beluga/motion/differential_drive_model.hpp

beluga/include/beluga/motion/omnidirectional_drive_model.hpp

beluga/include/beluga/motion/stationary_model.hpp

beluga/include/beluga/sensor/likelihood_field_model.hpp

beluga_amcl/src/amcl_node.cpp

beluga_example/config/params.yaml

Co-authored-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

glpuga

Minor comments, but LGTM!

beluga/include/beluga/algorithm/estimation.hpp

beluga/include/beluga/algorithm/particle_filter.hpp

beluga/include/beluga/algorithm/sampling.hpp

beluga/include/beluga/mixin/descriptor.hpp

beluga/include/beluga/mixin/utility.hpp

beluga/include/beluga/motion.hpp

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

ivanpauno

LGTM!

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

nahueespinosa added enhancement New feature or request cpp Related to C++ code labels Mar 3, 2023

nahueespinosa self-assigned this Mar 3, 2023

ivanpauno reviewed Mar 3, 2023

View reviewed changes

nahueespinosa requested a review from ivanpauno March 3, 2023 16:01

ivanpauno mentioned this pull request Mar 3, 2023

Compare performance between beluga_amcl and nav2_amcl #35

Closed

2 tasks

ivanpauno reviewed Mar 3, 2023

View reviewed changes

hidmic reviewed Mar 3, 2023

View reviewed changes

nahueespinosa requested a review from hidmic March 3, 2023 21:28

hidmic reviewed Mar 4, 2023

View reviewed changes

beluga/include/beluga/mixin.hpp Outdated Show resolved Hide resolved

beluga_amcl/src/amcl_node.cpp Outdated Show resolved Hide resolved

nahueespinosa force-pushed the nahuel/rehash-mixin-tags branch 3 times, most recently from f3f7a6b to 1ad5051 Compare March 6, 2023 22:56

nahueespinosa requested a review from hidmic March 6, 2023 23:59

Select mixin components at runtime

a81cabb

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

nahueespinosa force-pushed the nahuel/rehash-mixin-tags branch from 1ad5051 to a81cabb Compare March 7, 2023 01:18

hidmic reviewed Mar 7, 2023

View reviewed changes

nahueespinosa added 5 commits March 6, 2023 23:57

Use type aliases to improve readability

a1066d9

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

Add weights view to particle filter interface

c4f0b91

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

Move compose_interfaces to mixin header

44ec420

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

Tags are descriptors

4f702d4

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

Add some utility tests

f26f605

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

nahueespinosa force-pushed the nahuel/rehash-mixin-tags branch from f999460 to f26f605 Compare March 7, 2023 03:34

nahueespinosa added 2 commits March 7, 2023 00:42

Revert scope change in omnidirectional model

06038c7

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

Fix linter issues

a3a826c

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

nahueespinosa requested a review from hidmic March 7, 2023 12:09

Template template template parameters are not standard

cc99d92

`gcc` and `clang` compile them, but are technically not valid C++. Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

nahueespinosa force-pushed the nahuel/rehash-mixin-tags branch from 0ae1616 to cc99d92 Compare March 7, 2023 13:56

glpuga reviewed Mar 8, 2023

View reviewed changes

beluga_example/config/params.yaml Show resolved Hide resolved

nahueespinosa added 6 commits March 8, 2023 00:38

Use type aliasfor measurement interface

ef05cba

Also, take an rvalue reference. Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

Mixin are structs

3074cf6

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

Add stationary option to param descriptor

b4f2721

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

Add constants for string literals

6d21b4e

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

Only publish cloud if we have subscribers

f02a209

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

Add particle count to public interface

3a06321

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

nahueespinosa requested a review from glpuga March 8, 2023 12:12

Remove scope around publisher call

ddf6b0d

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

glpuga approved these changes Mar 8, 2023

View reviewed changes

ivanpauno mentioned this pull request Mar 10, 2023

Migrate beluga from ament_cmake to cmake #133

Merged

6 tasks

nahueespinosa added 3 commits March 11, 2023 18:48

More consistent use of type aliases

a326ebb

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

Add remaning tests

af7cb08

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

Update documentation, at last

ac773ce

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

ivanpauno approved these changes Mar 13, 2023

View reviewed changes

nahueespinosa and others added 3 commits March 13, 2023 18:37

Apply suggestions from code review

f3f47a7

Co-authored-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

Add alpha5 to example configuration

f2bc6cc

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

Merge branch 'main' into nahuel/rehash-mixin-tags

f8109da

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

glpuga approved these changes Mar 14, 2023

View reviewed changes

Address peer review comments

44576ff

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

ivanpauno approved these changes Mar 14, 2023

View reviewed changes

nahueespinosa added 2 commits March 14, 2023 10:30

Mixins and types with invariants are classes

e61ff15

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

Add missing public specifier

f55e338

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>

ivanpauno approved these changes Mar 14, 2023

View reviewed changes

nahueespinosa merged commit f50cc62 into main Mar 14, 2023

nahueespinosa deleted the nahuel/rehash-mixin-tags branch March 14, 2023 17:34

nahueespinosa mentioned this pull request Mar 16, 2023

Add system tests #138

Merged

6 tasks

glpuga mentioned this pull request Mar 30, 2023

CI runs are taking almost 14 minutes each #156

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Select mixin components at runtime #126

Select mixin components at runtime #126

nahueespinosa commented Mar 3, 2023 •

edited

Loading

nahueespinosa commented Mar 3, 2023

ivanpauno left a comment

ivanpauno left a comment

ivanpauno commented Mar 3, 2023

hidmic left a comment

hidmic left a comment

glpuga left a comment

ivanpauno commented Mar 8, 2023

nahueespinosa commented Mar 13, 2023

ivanpauno left a comment

glpuga left a comment

ivanpauno left a comment

Select mixin components at runtime #126

Select mixin components at runtime #126

Conversation

nahueespinosa commented Mar 3, 2023 • edited Loading

Summary

Checklist

nahueespinosa commented Mar 3, 2023

ivanpauno left a comment

Choose a reason for hiding this comment

ivanpauno left a comment

Choose a reason for hiding this comment

ivanpauno commented Mar 3, 2023

hidmic left a comment

Choose a reason for hiding this comment

hidmic left a comment

Choose a reason for hiding this comment

glpuga left a comment

Choose a reason for hiding this comment

ivanpauno commented Mar 8, 2023

nahueespinosa commented Mar 13, 2023

ivanpauno left a comment

Choose a reason for hiding this comment

glpuga left a comment

Choose a reason for hiding this comment

ivanpauno left a comment

Choose a reason for hiding this comment

nahueespinosa commented Mar 3, 2023 •

edited

Loading