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

fix python binding #987

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

fix python binding #987

wants to merge 6 commits into from

Conversation

pca006132
Copy link
Collaborator

@pca006132
Copy link
Collaborator Author

Not entirely sure why the test is failing. May need a docker to test this.

@pca006132
Copy link
Collaborator Author

I guess warp_batch was broken... will see how to fix that.

@pca006132
Copy link
Collaborator Author

In fact, I am not sure if we should keep the warp_batch API. I think this kind of modifying array by reference is quite brittle with transfer of ownership. Returning a new array will always work, and I don't think that will add too much overhead.

Copy link
Owner

@elalish elalish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning a new array sounds good to me.

pyproject.toml Show resolved Hide resolved
@pca006132
Copy link
Collaborator Author

Summary:

  1. No longer depends on unstable API. I found the solution to be surprisingly simple: just use the type caster for ndarray, it automatically performs conversion from ndarray to python handle (which was why we were trying to call ndarray_wrap).
  2. Changed the warp_batch API to require returning an array. I feel that we were lucky the previous code works, because it is probably relying on numpy not changing the underlying buffer for some operations.
  3. Added docstring override functionality, so we can customize some docstring. I think we may also want to add replacement function later, for example to replace different function names due to differences in naming convention.
  4. Download 2.2.0 of nanobind, and set the version string if we download it using FetchContent (nanobind is unavailable in python search path, so we cannot get the version string automatically in this case).
  5. Limit nanobind dependency to 2.2.0. I don't think we relied on unstable APIs now, but to avoid potential future build failure, it is better to specify the latest version that we support. As long as we have regular releases, we can still update nanobind version.

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.

Can not install manfold3d in docker image build on aarch64 using base image python:3.10-slim-bookworm
2 participants