-
Notifications
You must be signed in to change notification settings - Fork 0
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 python bindings #6
Conversation
826bb80
to
bd1b2a3
Compare
One idea I had one the weekend which might be a nice solution for allowing to add more comparison policies then the already implemented ones would be to add a template parameter pack and running
for each passed policy. I'll set this PR back to draft and test that out first as it would solve the issue with the missing tests (I think). |
Ok, that actually worked out quite well. I rearranged the bindings a little and added the missing unit tests. It is now possible to add a comparison policy as a template parameter to the |
3106fac
to
fd4a2fe
Compare
Rebased on current master. |
fd4a2fe
to
2d872aa
Compare
2d872aa
to
fbeec5a
Compare
Rebased to |
If pybind is available, this will compile the bindings, copy the python files to the build directory and add the test to ctest.
…separate namespaces.
More documentation
fbeec5a
to
3b5261d
Compare
Rebased to main |
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, thanks a lot for this contribution!
I guess, the advantage and usability for the Python end user is nice (much like the C++ interface).
But – one of the reasons this review took so long – getting there is honestly not that great of an experience 😅 – especially, if you're not (yet?) familiar with pybind etc.
Anyways, I guess it's still helpful, especially as you mainly mean it as a base to add Python support for the arbitration_graphs in order to integrate these in an Python-based evaluation framework. 🤞
And you did a great job in enriching these parts with good documentation and useful unit tests 👌
configure_file(${_py_test} ${PY_TEST_NAME} COPYONLY) | ||
|
||
add_test(NAME ${PY_TEST_NAME} | ||
COMMAND ${Python3_EXECUTABLE} ${PY_TEST_NAME} |
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.
CMake for Python… 🫣
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 know 😅
But I figured since you need to compile the bindings anyway, this is the easiest way to run the tests (they need to be run inside the build directory, because the compiled bindings are not installed or something like that).
Co-authored-by: Piotr Spieker <[email protected]>
Thanks for the review and the feedback! I know that the pybind stuff is a little hard to read at times (even though it's much better than the equivalent Boost.Python implementation 😂). However, since most of that stuff should never need to be seen or touched by a user of the library, it shouldn't matter too much. I'll rewrite the PR description quickly (since it's now used for the release notes), then merge this with a minor version bump. |
This contributes python bindings. Since this is a template C++ library, the python bindings have to be compiled at a later point in time, when the types are known. Therefore, we don't ship the bindings directly but rather add a convenience function that allows binding the types once the template arguments are known.
When pybind is available and
BUILD_TEST=TRUE
, a python module is built that allows running unit tests (almost) equivalent to the C++ ones.#minor