ANTsX / ANTsPy

A fast medical imaging analysis library in Python with algorithms for registration, segmentation, and more.
https://antspyx.readthedocs.io
Apache License 2.0
586 stars 161 forks source link

ENH: move c++ wrapping from pybind11 to nanobind #621

Closed ncullen93 closed 1 month ago

ncullen93 commented 1 month ago

This PR primarily moves the C++ wrapping framework from pybind11 to nanobind. This should improve the project by making the build process faster, the wheels smaller, and the initial "import ants" statement quicker. Moreover, any c++ functions that take in only an ants image are now overloaded so the can be called without appending pixel type + dimension suffixes to the name.

The c++ source code has now been moved outside of the python module folder, as it should be, and the package now relies on a modern pyproject.toml file instead of setup.py.

No user-facing functions have been change and internal python code has been minimally altered.

All tests pass locally on mac and on the github actions. However, I have not tested that a windows build would work because only the ITK and ANTs setup scripts for linux are currently called.

coveralls commented 1 month ago

Coverage Status

coverage: 80.524% (+0.1%) from 80.382% when pulling b3c95f219f6cdd10b0a341063b4ed84ae094c8ad on nanobuild-v2 into 150e6f698eb0560e7c83b6dda848d53feaf5e4e6 on master.

ncullen93 commented 1 month ago

@cookpa All the existing unit tests pass with the new wrapping and it should be good to go. The one thing I'm unsure of is the windows build. Particularly these lines https://github.com/ANTsX/ANTsPy/blob/cf801fc9e7203d52d1a5f667366a48097ddfcf27/CMakeLists.txt#L18

The CMakeLists.txt either needs to have some conditional statement inside it or the ants + itk setup scripts need to be moved to the pyproject.toml file to be run before the cmakelists.txt (like the current setup), again with conditional logic. The third option is to have the OS conditional logic inside the ants + itk setup scripts... that's maybe easiest.

cookpa commented 1 month ago

@cookpa All the existing unit tests pass with the new wrapping and it should be good to go. The one thing I'm unsure of is the windows build. Particularly these lines

https://github.com/ANTsX/ANTsPy/blob/cf801fc9e7203d52d1a5f667366a48097ddfcf27/CMakeLists.txt#L18

The CMakeLists.txt either needs to have some conditional statement inside it or the ants + itk setup scripts need to be moved to the pyproject.toml file to be run before the cmakelists.txt (like the current setup), again with conditional logic. The third option is to have the OS conditional logic inside the ants + itk setup scripts... that's maybe easiest.

I think CMake can do it:

if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
  execute_process(COMMAND cmd /c ${PROJECT_SOURCE_DIR}/scripts/configure_ITK.bat)
else
  execute_process(COMMAND bash ${PROJECT_SOURCE_DIR}/scripts/configure_ITK.sh)
endif()
cookpa commented 1 month ago

Docker builds too on Linux

ncullen93 commented 1 month ago

Ok great, thanks. I will add that.