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

Fix runtime errors, fix typo, add dockerfile #3

Merged
merged 10 commits into from
Mar 18, 2024
Merged

Conversation

changh95
Copy link
Contributor

@changh95 changh95 commented Mar 15, 2024

@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

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

python/map_closures/pipeline.py Show resolved Hide resolved
python/map_closures/pipeline.py Outdated Show resolved Hide resolved
python/map_closures/pipeline.py Outdated Show resolved Hide resolved
python/map_closures/pipeline.py Outdated Show resolved Hide resolved
@changh95
Copy link
Contributor Author

changh95 commented Mar 15, 2024

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!

@saurabh1002 saurabh1002 merged commit 248553d into PRBonn:main Mar 18, 2024
@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
Labels
bug Something isn't working documentation Improvements or additions to documentation python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants