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

Refactor resampling policies into filter update control #233

Merged
merged 1 commit into from
Jul 1, 2023

Conversation

glpuga
Copy link
Collaborator

@glpuga glpuga commented Jun 20, 2023

Proposed changes

  • For the time being gives up on the hope to find a meaningful overarching common interface between different filter update policies from the available three samples, and instead deal with all three as one-offs.
  • Relocates the previous PolicyPoller and the three policies into beluga_amcl, since their design is strongly influenced by us trying to replicate Nav2's behavior.
  • Rewrites the poller into a broader FilterUpdateControlMixin class that encapsulates Nav2's filter update behavior.
  • Adds a customization point to the main filter aggregated classes, allowing the user to add a mixin to customize the filter behavior an interface. Uses that to roll FilterUpdateControlMixin into the filter in beluga_amcl.
  • Refactors the transform and pose publication behaviors in beluga_amcl, since they did not match Nav2's and the latter's behavior is very related to the filter control policies above.

I still think the three policies (not the FilterUpdateControlMixin) can eventually go back into the main beluga library, but I don't think forcing a common abstraction on them will be meaningful.

Type of change

  • 🐛 Bugfix (change which fixes an issue)
  • 🚀 Feature (change which adds functionality)
  • 📚 Documentation (change which fixes or extends documentation)

💥 Breaking change! Explain why a non-backwards compatible change is necessary or remove this line entirely if not applicable.

Checklist

Put an x in the boxes that apply. This is simply a reminder of what we will require before merging your code.

  • Lint and unit tests (if any) pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • All commits have been signed for DCO

Additional comments

Preliminary performance comparison with Nav2 looks now much better. Beam model, Beluga vs. Nav2 AMCL:

beam_beluga_log

Likelihood model, Beluga vs. Nav2 AMCL:

likelihood_beluga_log

@glpuga glpuga force-pushed the glpuga/put_resampling_policies_in_amcl_scope branch from 303b6a0 to 3a9b3f0 Compare June 20, 2023 15:03
@glpuga glpuga marked this pull request as draft June 20, 2023 15:12
@nahueespinosa nahueespinosa added enhancement New feature or request cpp Related to C++ code labels Jun 21, 2023
@glpuga glpuga force-pushed the glpuga/put_resampling_policies_in_amcl_scope branch 3 times, most recently from 68a6ada to 7d15fb8 Compare June 24, 2023 22:46
@glpuga glpuga marked this pull request as ready for review June 24, 2023 22:47
@glpuga glpuga force-pushed the glpuga/put_resampling_policies_in_amcl_scope branch from 7d15fb8 to d4f47dd Compare June 24, 2023 23:41
Copy link
Member

@nahueespinosa nahueespinosa left a comment

Choose a reason for hiding this comment

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

LGTM!

@hidmic
Copy link
Collaborator

hidmic commented Jun 28, 2023

Looks like tests migrating from beluga to beluga_amcl reduced marginally reduced beluga coverage.

@glpuga
Copy link
Collaborator Author

glpuga commented Jun 28, 2023

Looks like tests migrating from beluga to beluga_amcl reduced marginally reduced beluga coverage.

Indeed. The format tests in the new package require to spread a long line into multiple lines, and therefore reduced the coverage as measured by the line count.

@glpuga glpuga force-pushed the glpuga/put_resampling_policies_in_amcl_scope branch 2 times, most recently from d6a7ce2 to be3be4e Compare July 1, 2023 18:45
Signed-off-by: Gerardo Puga <glpuga@ekumenlabs.com>
@glpuga glpuga force-pushed the glpuga/put_resampling_policies_in_amcl_scope branch from be3be4e to 27f857b Compare July 1, 2023 18:56
@glpuga glpuga merged commit 12d69be into main Jul 1, 2023
7 of 8 checks passed
@glpuga glpuga deleted the glpuga/put_resampling_policies_in_amcl_scope branch July 1, 2023 19:30
@glpuga glpuga mentioned this pull request Sep 3, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp Related to C++ code enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants