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 Graph SLAM documentation and SE(2) example #302

Merged
merged 1 commit into from Mar 25, 2020

Conversation

JeffLIrion
Copy link
Contributor

@JeffLIrion JeffLIrion commented Mar 1, 2020

This is a work in progress.

The Jupyter notebook doesn't render correctly on GitHub, so I moved the write-up into a LaTeX file and included both the .tex source and the generated PDF. I'll probably remove it altogether from the Jupyter notebook.

TODO: Add code and example problem

Related issue: #296

@AtsushiSakai
Copy link
Owner

@JeffLIrion Your doc looks great!!. Please keep working it : ).

@lgtm-com
Copy link

lgtm-com bot commented Mar 7, 2020

This pull request introduces 1 alert when merging 4e2d7a8 into 869650f - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Mar 8, 2020

This pull request introduces 1 alert when merging a12b675 into 869650f - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Mar 8, 2020

This pull request introduces 1 alert when merging 6ebdb42 into 869650f - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@JeffLIrion
Copy link
Contributor Author

Current status

  • Add a few citations to the write-up
  • Add code for loading, optimizing, and plotting SE(2) datasets
  • Add an example problem with figures
  • Clean up the Jupyter notebook

Any other tasks that you'd like to see completed?

@AtsushiSakai
Copy link
Owner

AtsushiSakai commented Mar 9, 2020

Thank you for your hard work!!. That is OK. Please let me know when you are ready to start to review.

@JeffLIrion JeffLIrion changed the title [DRAFT] Add Graph SLAM documentation and example Add Graph SLAM documentation and SE(2) example Mar 14, 2020
@JeffLIrion JeffLIrion marked this pull request as ready for review March 14, 2020 01:53
@JeffLIrion
Copy link
Contributor Author

This is ready to start the review process!

@AtsushiSakai AtsushiSakai self-requested a review March 15, 2020 06:23
Copy link
Owner

@AtsushiSakai AtsushiSakai left a comment

Choose a reason for hiding this comment

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

Thank you for your big work. I have several questions.

  1. This repo already have graph_based_slam.py. is your code using same algorithm?
  2. Do you want to keep latex codes and pdf? is it impossible to use jupyter notebook to describe math?
  3. You added images in the image dir. Are these impossible to embedded a jupyter notebook?

@JeffLIrion
Copy link
Contributor Author

JeffLIrion commented Mar 15, 2020

  1. This repo already have graph_based_slam.py. is your code using same algorithm?

    • It looks to be the same algorithm: compute b and H, solve the linear system H dx = -b, and update the poses. What the included package contributes is a Graph SLAM solver that:

      • uses a standard data format (.g2o files) for importing and exporting data
      • has been validated against results from g2o, which is arguably the de facto Graph SLAM solver
    • If the code in graph_based_slam.py could be used to achieve these same results, then I would agree that the code is redundant.

  2. Do you want to keep latex codes and pdf? is it impossible to use jupyter notebook to describe math?

  3. You added images in the image dir. Are these impossible to embedded a jupyter notebook?

    • I can embed them in a Jupyter notebook. I also meant to make a .gif out of them. I'll do that now.
    • Follow-up: I created the .gif image and embedded it in the Jupyter notebook. Let me know if you'd like me to delete the .png images.

@JeffLIrion
Copy link
Contributor Author

For my own reference:

convert -background white -alpha remove -alpha off -delay 200 -loop 0 iteration*.png Graph_SLAM_optimization.gif

@AtsushiSakai
Copy link
Owner

AtsushiSakai commented Mar 23, 2020

If the code in graph_based_slam.py could be used to achieve these same results, then I would agree that the code is redundant.

Do you think it is possible for you to update the graph_based_slam.py using your solver?

@AtsushiSakai
Copy link
Owner

Yes, I definitely think that both the LaTeX source and the PDF should be kept.

OK. I got it.

@AtsushiSakai
Copy link
Owner

Let me know if you'd like me to delete the .png images.

Oh. great. Please remove the pngs. I want to keep the PR as simple as possible.

@JeffLIrion
Copy link
Contributor Author

Let me know if you'd like me to delete the .png images.

Oh. great. Please remove the pngs. I want to keep the PR as simple as possible.

Done. I left only the .gif image.

@JeffLIrion
Copy link
Contributor Author

If the code in graph_based_slam.py could be used to achieve these same results, then I would agree that the code is redundant.

Do you think it is possible for you to update the graph_based_slam.py using your solver?

I'm sure it's possible, but my wife and I just had a baby, so I simply don't have the time to do that right now. Sorry.

@AtsushiSakai
Copy link
Owner

AtsushiSakai commented Mar 24, 2020

OK. Thank you so much.

I simply don't have the time to do that right now. Sorry.

No problem. Is it ok to merge this PR now? Maybe I can do it later.

@JeffLIrion
Copy link
Contributor Author

No problem. Is it ok to merge this PR now? Maybe I can do it later.

I'll squash my commits and then it will be ready.

@JeffLIrion
Copy link
Contributor Author

I squashed the commits, so this is ready to be merged.

@AtsushiSakai AtsushiSakai merged commit 55478fa into AtsushiSakai:master Mar 25, 2020
@AtsushiSakai
Copy link
Owner

@JeffLIrion Thank you so much for your great work. More PRs are always welcome!!

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.

None yet

2 participants