-
Notifications
You must be signed in to change notification settings - Fork 544
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
[WIP] feature/python_module : Adding libpointmatcher's Python bindings (#222) #397
[WIP] feature/python_module : Adding libpointmatcher's Python bindings (#222) #397
Conversation
717c92e
to
84c322f
Compare
84c322f
to
c8fd787
Compare
315d98c
to
b80b5ac
Compare
The python module now only link with pointmatcher instead of building it. The python module can now be installed.
840b003
to
9f4d0c9
Compare
9476203
to
f8273ca
Compare
f8273ca
to
6b909ea
Compare
Also fixed a typo in the IO.cpp file
Removed the bindings for ParametersDoc and Parameters in parametrizable.cpp
In order to get more consistent tests results with the Python version examples.
Added the suffix _const at the end of the method signature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @AlexandreG87,
This PR looks really cool. Just left a few minor comments.
python/pypointmatcher_helper.h
Outdated
@@ -0,0 +1,72 @@ | |||
#ifndef PYTHON_PYPOINTMATCHER_HELPER_H | |||
#define PYTHON_PYPOINTMATCHER_HELPER_H | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of this file is not very clear. To me it is more like a centralized typedefs-header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it ended up being that way, but in the main purpose of this file was to regroup all the necessary include in one place and also the PYBIND11_MAKE_OPAQUE
macro declaration as mentioned in pybind11 documentation. I thought about whether I put these aliases just before their use, like the io.cpp source file, or in a common header file like it is right now. Some of them are used across some other files, so I did this to avoid re-declaring them every time I needed it. But, if you suggest that it would be better to declare it only where it is needed (saying that make me realize that it makes much more sense to do so 😅), I have no problem to fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey. For now I think it's ok to keep the file. But I would rename it to pypointmatcher_typedefs
or pypointmatcher_types
. It would be easier to understand its content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I'll see what I can do about that.
@YoshuaNava Thank you for the feedback and I hope you will be satisfied with my answers. 😄 |
Now giving the possibility to choose between system installed pybind11 and a git sub-module by setting USE_SYSTEM_PYBIND11 variable.
Also, making PLYProperties and PLYDescPropMap typedefs opaque in order to avoid unwanted behavior.
Based on PR norlab-ulaval#397 feedback
A copy was made every time it was called because no return value policy was specified.
I am, thank you for addressing all of them. Really cool work ⚡ I left just a few minor comments. Most of the items are resolved in my opinion. Before approving I have some final questions:
|
Prevent pybind11 to convert them into Python's list and dict.
* Added some documentation * Switched to opaque container Parameters
@AlexandreG87 What is the status of this PR?
|
Hi @YoshuaNava, I would say that it's not quite finished yet. I'll start to address your comments first, then we'll see what needs to be done to make it better.
Thanks, it's very appreciated. Maybe, I'll give it a try too.
It was not in my plans, I'll need to check with @pomerlef what can be done about that. |
|
@AlexandreG87 Regarding your answers:
Oki.
I would say that for minimum support we should make sure that the ROS build still runs. In a separate PR I plan to submit a new CMake setup, that allows you to have ROS CI/CD, running tests, and at the same time build without catkin, in case wanted. I'll open an issue to propose it.
That's good 👍
Very good. No more comments from my side apart from the open ones. |
Based on PR norlab-ulaval#397 feedback
I would go on an merge this PR and open issues if something go wrong. For now, I don't have time to test it on MacOs. |
Examples use float instead of double
@pomerlef There were some more things that I wanted to do before you merge this PR and I will do them as soon as possible. |
@pomerlef You can proceed to the merge when you're ready. |
Here are the python's bindings for libpointmatcher as requested in the issue #222.
Things to do before merging :
Make sure that the module get installed in the sites-packages of the current Python environment (maybe using pip install?).
Provide a tutorial on how to compile and install the module.
Provide a tutorial on how to use it.
Finish to transpose the C++ examples into Python.
Merging PR Replaced the remaining raw pointers with shared pointers in Registrar.h #395
Make sure that the bindings compile on macOS?
Testing :
If you want to test the Python module on your side, clone my libpointmatcher's fork. Then, follow the compilation and installation instructions from this tutorial.
I've tested it with g++ 7.5.0
and clang++ 10.0.0on Ubuntu 18.04.Edit: The bindings no longer compiles on clang++ 10.0, and on clang++ 6.0 either, since the adding of the IO module.
Feedback on the bindings themselves and the tutorials will be really appreciated, thanks! 🙂
Last edited : 20 august 2020