Skip to content

Evaluating new clustering + sensor model #123

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

glpuga
Copy link
Collaborator

@glpuga glpuga commented Feb 28, 2025

Proposed changes

Note: this needs Ekumen-OS/beluga#476 to be merged before, because it assumes the new sensor model to be available.

This PR merges into the main Beluga evaluation report changes created to evaluate the impact of Ekumen-OS/beluga#476

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)

Additional comments

Anything worth mentioning to the reviewers.

<param name="use_sim_time" value="$(var use_sim_time)" />
</node> -->

</launch>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

end of line.

@glpuga glpuga force-pushed the glpuga/join_florencias_version_with_main branch 4 times, most recently from 161941a to f16f08b Compare March 20, 2025 21:03
@glpuga glpuga force-pushed the glpuga/join_florencias_version_with_main branch from f16f08b to 7399f6d Compare March 20, 2025 21:09
@glpuga
Copy link
Collaborator Author

glpuga commented Mar 20, 2025

This PR follows the tests and updates used to teset Ekumen-OS/beluga#476 (comment)

Unfortunately, the PR now has conflicts with main that would be unwise to fix in a hurry given that testing the changes would require at least doing a partial run and rebuild of the testbench, so this will have to wait until I'm back to move forward.

Regardless, this needs Ekumen-OS/beluga#476 to be merged before being merged, because it assumes the new sensor model to be available.

@glpuga glpuga marked this pull request as ready for review March 20, 2025 21:13
@glpuga
Copy link
Collaborator Author

glpuga commented Mar 20, 2025

This is the report produced by this version (except for this report the 24hs dataset was not used):
report.pdf

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.

Overall LGTM

@@ -1,80 +1,227 @@
<launch>
<arg name="map_filename"/>
<arg name="map_filename" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

@glpuga meta: why adding these spaces? Google yields some XHTML connection to this formatting style that I don't think is relevant here.

beluga_amcl_likelihood_cpu_util = "???"
beluga_amcl_likelihood_peak_rss = "???"
beluga_amcl_likelihood_prob_cpu_util = "???"
beluga_amcl_likelihood_prob_peak_rss = "???"
Copy link
Collaborator

Choose a reason for hiding this comment

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

@glpuga meta: we really need a better way to deal with partial benchmark runs than these huge try...except blocks.

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.

2 participants