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

Tiziano/build linear system #249

Merged
merged 4 commits into from
Oct 27, 2023
Merged

Conversation

tizianoGuadagnino
Copy link
Collaborator

Reach branch name a.k.a expose the linear system construction in the optimization loop. Allows to compute the covariance of the estimate and use the KISS-ICP code for other tasks (LIO etc).

@tizianoGuadagnino tizianoGuadagnino self-assigned this Oct 27, 2023
@tizianoGuadagnino tizianoGuadagnino requested review from nachovizzo and benemer and removed request for nachovizzo and benemer October 27, 2023 10:48
@tizianoGuadagnino tizianoGuadagnino merged commit da1bff1 into main Oct 27, 2023
17 checks passed
@tizianoGuadagnino tizianoGuadagnino deleted the tiziano/build_linear_system branch October 27, 2023 10:55
@nachovizzo
Copy link
Collaborator

@tizianoGuadagnino this looks good to me. But for next time is better if we go through a round of reviews before merging this tho the main branch immediately.

  1. Since this is not even mentioned in the paper should we at least add some comments?
  2. Why you have not exposed this to the python API, the ROS wrappers etc 🤔? This now diverged from the original way of implementing new features

@tizianoGuadagnino
Copy link
Collaborator Author

@nachovizzo there is no difference with what was there before, even less difference with the paper because we never mention exponential and logarithmic mapping in the alignment part...

The RegisterFrame function has been simply splitted in two, I'm happy to revert it if that breaks the flow of the development.

How should I expose it in the ROS wrapper ? please explain. I'm happy to pybinded if that is critical.

I follow the policy "It is a small change for now I merge it, I'm happy to revert it later" cit someone

@nachovizzo
Copy link
Collaborator

@tizianoGuadagnino Honestly I haven't reviewed the changes thoroughly :D

Now I see that API hasn't been changed (GitHub diff is not helping with this). But since you opened a new version I assumed you were messing around with the API (check https://semver.org/)

Since the API didn't change I wouldn't bump a new version, and if the BuildLinearSystem function is not exposed then I will keep it an unnamed namespace within the translation unit....

If it requires to be exposed, then that's what would be maybe interesting to expose to Python and ROS

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