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

Feature/lifelong compression #2

Open
wants to merge 83 commits into
base: foxy-devel
Choose a base branch
from

Conversation

kmilo7204
Copy link

@kmilo7204 kmilo7204 commented Nov 3, 2021

Basic info

Information-Theoretic compression of Pose Graphs for Laser-Based SLAM.

Description of contribution in a few bullet points

  • Added theoretic information functionality to find the laser range scan (In a set of laser range scans) which provides the less map's information.
  • Documents
  • Follow this link to access the Thesis document.
  • Follow this link to access the paper.

Future work that may be required in bullet points

  • Data structures and function improvements.

Current state

  • Code refactoring
  • Tests
  • SLAM toolbox modifications

@kmilo7204 kmilo7204 requested a review from hidmic November 3, 2021 16:28
@hidmic
Copy link
Collaborator

hidmic commented Nov 3, 2021

@kmilo7204 I have the impression this is targeting the wrong branch -- quite a few commits in there are not yours.

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 @kmilo7204 !

src/experimental/slam_toolbox_lifelong.cpp Outdated Show resolved Hide resolved
src/experimental/slam_toolbox_lifelong.cpp Outdated Show resolved Hide resolved
src/experimental/slam_toolbox_lifelong.cpp Outdated Show resolved Hide resolved
src/experimental/slam_toolbox_lifelong.cpp Outdated Show resolved Hide resolved
src/experimental/slam_toolbox_lifelong.cpp Outdated Show resolved Hide resolved
src/experimental/slam_toolbox_lifelong.cpp Outdated Show resolved Hide resolved
@kmilo7204 kmilo7204 changed the base branch from ros2 to foxy-devel November 4, 2021 15:32
@kmilo7204
Copy link
Author

kmilo7204 commented Nov 17, 2021

From my last discussion with @hidmic.

I am sharing a brief analysis of the Algorithm 1, this is my interpretation of it. From my POV the algorithm is confusing at some point, specially in the calculation of probabilities (Even when equation are really clear there), but based on the number of calculations I ended up with the right values to calculate.

I made calculations for two observations (Not with numbers but keeping the variable representation), you can find it in the attached document.

Algorithm 1 analysis.pdf

I will be moving from this point now.

@nahueespinosa
Copy link

nahueespinosa commented Dec 2, 2021

@kmilo7204 Thanks for the analysis!

I need to clarify something. My interpretation would be the following:

Let's say that after applying Algorithm 1 to a set of 3 measurements, one possible outcome z' is 2 free, 1 not observed with 15% chance.

In order to compute the entropy of C given that particular outcome we would do:

And we would need to repeat that for every possible outcome in the resulting hashtable from Algorithm 1 to obtain the mutual information.

@kmilo7204
Copy link
Author

kmilo7204 commented Dec 2, 2021

@nahueespinosa

You are right about the clarification. I will correct the document so it is available for all of us!

Thanks for the clarification and comments!

@hidmic hidmic self-requested a review August 2, 2022 15:11
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.

Quick partial pass (as I see the old approach is still here).

.vscode/.gitignore Outdated Show resolved Hide resolved
src/experimental/information_estimates.cpp Outdated Show resolved Hide resolved
src/experimental/information_estimates.cpp Outdated Show resolved Hide resolved
src/experimental/information_estimates.cpp Outdated Show resolved Hide resolved
src/experimental/information_estimates.cpp Outdated Show resolved Hide resolved
src/experimental/information_estimates.cpp Outdated Show resolved Hide resolved
src/experimental/information_estimates.cpp Outdated Show resolved Hide resolved
Signed-off-by: Camilo Andres Alvis Bautista <kmilo7204@gmail.com>
Signed-off-by: Camilo Andres Alvis Bautista <kmilo7204@gmail.com>
Signed-off-by: Camilo Andres Alvis Bautista <kmilo7204@gmail.com>
Signed-off-by: Camilo Andres Alvis Bautista <kmilo7204@gmail.com>
Signed-off-by: Camilo Andres Alvis Bautista <kmilo7204@gmail.com>
Signed-off-by: Camilo Andres Alvis Bautista <kmilo7204@gmail.com>
Signed-off-by: Camilo Andres Alvis Bautista <kmilo7204@gmail.com>
Signed-off-by: Camilo Andres Alvis Bautista <kmilo7204@gmail.com>
@kmilo7204
Copy link
Author

@hidmic
I think the core functionality is ready for review. I am almost done with the test (still figuring out something), but I think we have a good amount of code to review.

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.

Nth pass :) Besides some oddities in the way the code is written, first impressions suggest it is correct. Did performance improve @kmilo7204 ?

src/experimental/utils.cpp Outdated Show resolved Hide resolved
src/experimental/utils.cpp Outdated Show resolved Hide resolved
src/experimental/utils.cpp Outdated Show resolved Hide resolved
src/experimental/utils.cpp Outdated Show resolved Hide resolved
Comment on lines 119 to 120
karto::Vector2<kt_double> calculateCellIntersectionPoints(karto::Vector2<kt_double> const & laser_start,
karto::Vector2<kt_double> const & laser_end, karto::Vector2<kt_double> const & cell_start, karto::Vector2<kt_double> const & cell_end)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kmilo7204 meta: I wonder, quite a few functions take segments and bounding boxes as a pair of 2D points (endpoints and corners, respectively). Why not adding Segment2 and Box2 structs for improved readability?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for this suggestion.

From my perspective it is only worth the implementation of Segment2, because this kind of data structure is used a lot within this code. It was defined as a template structure in utils.hpp as shown next:

    template<typename T>
    struct Segment2
    {
        karto::Vector2<T> start;
        karto::Vector2<T> end;
    };

Regarding the Box2 I decided to not implement that structure for two reasons.

  1. It is used only a couple of times, to pass the cell corners to find the intersection points between a laser beam and the current cell.
  2. Sometimes and depending on the where the cell is located WRO the laser beam origin we need to modify the corners. So I found that it might be confusing to handle the changes with the Box2 structure. Please consider the following structure definition (Maybe the naming convention is what seems confusing for me):
    template<typename T>
    struct Box2
    {
        karto::Vector2<T> bl; // bottom left
        karto::Vector2<T> br; // bottom right
        karto::Vector2<T> tl; // top left
        karto::Vector2<T> tr; // top right
    };

I will leave this thread open!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kmilo7204 hmm, I don't follow why Box2 definition requires coordinates for each corner. Unless It's a four sided polygon, coordinates for two corners along a diagonal should always suffice.

src/experimental/information_estimates.cpp Outdated Show resolved Hide resolved
src/experimental/information_estimates.cpp Outdated Show resolved Hide resolved
src/experimental/information_estimates.cpp Outdated Show resolved Hide resolved
src/experimental/information_estimates.cpp Outdated Show resolved Hide resolved
@kmilo7204
Copy link
Author

Nth pass :) Besides some oddities in the way the code is written, first impressions suggest it is correct. Did performance improve @kmilo7204 ?

@hidmic Yes performance improved a lot. Prior to this iteration we were calculating the mutual information for 7 scans (At max), now we are processing 19 or 20 scans and obtaining the same time.

This is a decent number for this calculations, but I think we can improve it a little bit more.

…optional return values for some functions

Signed-off-by: Camilo Andres Alvis Bautista <kmilo7204@gmail.com>
Signed-off-by: Camilo Andres Alvis Bautista <kmilo7204@gmail.com>
Signed-off-by: Camilo Andres Alvis Bautista <kmilo7204@gmail.com>
Signed-off-by: Camilo Andres Alvis Bautista <kmilo7204@gmail.com>
Signed-off-by: Camilo Andres Alvis Bautista <kmilo7204@gmail.com>
Signed-off-by: Camilo Andres Alvis Bautista <kmilo7204@gmail.com>
Signed-off-by: Camilo Andres Alvis Bautista <kmilo7204@gmail.com>
Signed-off-by: Camilo Andres Alvis Bautista <kmilo7204@gmail.com>
…ributes

Signed-off-by: Camilo Andres Alvis Bautista <kmilo7204@gmail.com>
@hidmic
Copy link
Collaborator

hidmic commented Jan 30, 2023

@kmilo7204 what happened here? How far are we with this?

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.

3 participants