Skip to content

Fix runtime errors, fix typo, add dockerfile #3

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.

Sign up for GitHub

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

Jump to bottom
Merged
saurabh1002 merged 10 commits into from
Mar 18, 2024
Merged

Fix runtime errors, fix typo, add dockerfile #3

saurabh1002 merged 10 commits into from
Mar 18, 2024

Conversation

@changh95
Copy link
Contributor

@changh95 changh95 commented Mar 15, 2024 •
edited
Loading

@saurabh1002 and rest of the PR Bonn team, thank you very much for making nice software!

I'm submitting a PR based on 3 changes

  • Fixing a minor typo
  • Fixing 2 runtime errors
  • Adding a dockerfile
    • This might be a personal preference, but I've seen several cases avoiding installing OpenCV in the system, as it sometimes crashes with other versions of OpenCV or other projects. Docker is often used to make an isolated environment to solve this kind of issue, so I've packaged the entire MapClosures in a Docker image.
    • However, this change might go against the idea of KISS-ICP being super easy to use with just pip3 install kiss-icp, so it's totally understandable if you think this change shouldn't be merged into the main branch :)

Output

Works well on Docker :)

image

@saurabh1002
Copy link
Collaborator

saurabh1002 commented Mar 15, 2024 •
edited
Loading

Thanks a lot @changh95 for using our code and submitting Issue and a PR so fast. I will merge it soon, just leaving some comments about it below, and request you to make these changes before I merge.

  1. Regarding Issue CLI replication when creating local maps #1, it was just a remaining print statement that I forgot to delete, you can just delete that line instead of commenting it out.
  2. Sorry for Issue Missing parameters when self._eval == False #2, just some negligence on my side, will merge your PR with some modifications suggested on the file
  3. We will provide a docker support very soon, so I am not merging the Dockerfile and related updates in README from Fix runtime errors, fix typo, add dockerfile #3

saurabh1002
saurabh1002 requested changes Mar 15, 2024
View reviewed changes
@changh95
Copy link
Contributor Author

changh95 commented Mar 15, 2024 •
edited
Loading

Applied comments as requested, @saurabh1002
Shall I remove the docker image from the branch? or do you have access to my commit history to remove the docker-related commit?

EDIT: I removed the Dockerfile!

@changh95 changh95 requested a review from saurabh1002 March 15, 2024 08:53
@ga58lar ga58lar mentioned this pull request Mar 15, 2024
Merged
@saurabh1002 saurabh1002 merged commit 248553d into PRBonn:main Mar 18, 2024
This was referenced Mar 18, 2024
Closed
Closed
@saurabh1002 saurabh1002 added bug Something isn't working documentation Improvements or additions to documentation python labels Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@saurabh1002 saurabh1002 Awaiting requested review from saurabh1002 saurabh1002 is a code owner

Assignees

No one assigned

Labels

bug Something isn't working documentation Improvements or additions to documentation python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@changh95 @saurabh1002