Port from pybind11 to nanobind#34
Conversation
There was a problem hiding this comment.
Pull request overview
Ports the C++/Python bindings from pybind11 to nanobind, updating build configuration and exposing nanobind-backed array/ndarray interfaces (incl. array API / DLPack-related hooks).
Changes:
- Switch CMake build from
pybind11_add_moduletonanobind_add_moduleand update dependency discovery. - Replace pybind11 binding code with nanobind equivalents across all exposed Atlas types.
- Introduce nanobind ndarray-based views for
Field(host/device access + NumPy/CuPy/DLPack entry points).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/atlas4py/CMakeLists.txt |
Updates build to find/link nanobind and build the extension with nanobind_add_module. |
src/atlas4py/_atlas4py.cpp |
Rewrites the binding layer for nanobind and adds ndarray/array interop for fields. |
pyproject.toml |
Switches build-system requirement from pybind11 to nanobind. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @tehrengruber this should now be ready for you to review. Thanks! |
wdeconinck
force-pushed
the
nanobind
branch
2 times, most recently
from
1f4b632 to
503f6d4
Compare
tehrengruber
left a comment
tehrengruber
left a comment
There was a problem hiding this comment.
Thanks, only minor things.
| case DataType::KIND_UINT64: | ||
| return "uint64"; | ||
| default: | ||
| return ""; |
There was a problem hiding this comment.
Assuming the default case is a bug, not wrong user input
| return ""; | |
| assert(False && "Unexpected DataType") | |
| std::abort(); |
There was a problem hiding this comment.
I added
throw nb::type_error("Unexpected DataType");There was a problem hiding this comment.
My reasoning was this is an impossible case so I didn't want to make this except and allow the compiler to deduce this case not reachable. Probably overkill for a dtype deduction...
| }, nb::rv_policy::reference_internal) | ||
| // Numpy array interface, see https://numpy.org/doc/stable/reference/arrays.interface.html | ||
| .def("__array__", [](Field &self, nb::handle dtype, nb::handle copy) { | ||
| return make_ndarray<nb::numpy>(self, MemorySpace::host); |
There was a problem hiding this comment.
An assert that dtype and copy is indeed None is missing, otherwise this is silently assumed.
There was a problem hiding this comment.
Should be handled in 4c4ee65 including a test ; my comment got lost.
|
Thanks @tehrengruber for your excellent review. I believe I have addressed all suggestions now. Keep in mind this particular PR is stacked on #36 (Fix editable installs), which is not yet reviewed. |
|
This is not addressed right?: #34 (comment)
Ah right, I missed this since the PR number is higher. I put some comments there. Thanks for the work! |
This PR ports the pybind11 implementation to use nanobind instead.
Unit tests had already been improved with previous PR #32, to catch errors in porting, and are untouched here.
Especially noteworthy here are the new array-view interfaces which nanobind brings more naturally, with support for DLPACK etc.