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 NDT AMCL node and tests. #347

Merged
merged 16 commits into from
May 10, 2024
Merged

Add NDT AMCL node and tests. #347

merged 16 commits into from
May 10, 2024

Conversation

serraramiro1
Copy link
Contributor

@serraramiro1 serraramiro1 commented May 6, 2024

Proposed changes

Adds a new node to leverage the NDT sensor model.
Closes #312

Type of change

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

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

Signed-off-by: Ramiro Serra <serraramiro@ekumenlabs.com>
Signed-off-by: Ramiro Serra <serraramiro@ekumenlabs.com>
@serraramiro1 serraramiro1 mentioned this pull request May 6, 2024
7 tasks
Copy link
Collaborator

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

First pass. CI does not look happy.

beluga_amcl/src/ndt_amcl_node.cpp Outdated Show resolved Hide resolved
beluga_amcl/test/test_ndt_amcl_node.cpp Show resolved Hide resolved
beluga_amcl/src/ndt_amcl_node.cpp Show resolved Hide resolved
Signed-off-by: Ramiro Serra <serraramiro@ekumenlabs.com>
@serraramiro1 serraramiro1 added enhancement New feature or request cpp Related to C++ code labels May 7, 2024
@serraramiro1 serraramiro1 requested a review from hidmic May 7, 2024 14:50
Signed-off-by: Ramiro Serra <serraramiro@ekumenlabs.com>
Signed-off-by: Ramiro Serra <serraramiro@ekumenlabs.com>
Signed-off-by: Ramiro Serra <serraramiro@ekumenlabs.com>
@serraramiro1
Copy link
Contributor Author

First pass. CI does not look happy.

Although this is a regression in terms of coverage, I feel like the line coverage looks sane.
Do you mind TAL and providing insight whether the current state is enough or not?

I feel like >90% is good enough for nodes

@hidmic
Copy link
Collaborator

hidmic commented May 8, 2024

Do you mind TAL and providing insight whether the current state is enough or not?

If codecov line coverage report is accurate, a test with non-empty laserscan and a non-empty particle cloud would exercise the missing code paths. You could trick it by de-duplicating code too.

I feel like >90% is good enough for nodes

I'm reluctant to let this one slip.

Signed-off-by: Ramiro Serra <serraramiro@ekumenlabs.com>
Copy link
Collaborator

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM though I still see a sensible decrease in test coverage.

beluga_amcl/include/beluga_amcl/common.hpp Outdated Show resolved Hide resolved
beluga_amcl/test/cmake/ament.cmake Show resolved Hide resolved
@hidmic
Copy link
Collaborator

hidmic commented May 9, 2024

/home/runner/work/beluga/beluga/beluga_amcl/include/beluga_amcl/ndt_amcl_node.hpp:91: error: Compound beluga_amcl::NdtAmclNode is not documented. (warning treated as error, aborting now)

build-docs job isn't happy :)

Signed-off-by: Ramiro Serra <serraramiro@ekumenlabs.com>
Signed-off-by: Ramiro Serra <serraramiro@ekumenlabs.com>
Signed-off-by: Ramiro Serra <serraramiro@ekumenlabs.com>
Signed-off-by: Ramiro Serra <serraramiro@ekumenlabs.com>
@serraramiro1 serraramiro1 merged commit 952eb30 into main May 10, 2024
7 of 9 checks passed
@serraramiro1 serraramiro1 deleted the ramiro/ndt_amcl_node branch May 10, 2024 22:19
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.

Implement 2D NDT-MCL ROS 2 node
2 participants