8000 fix python binding by pca006132 · Pull Request #987 · elalish/manifold · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix python binding #987

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Oct 18, 2024
Merged

fix python binding #987

merged 6 commits into from
Oct 18, 2024

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
8000 pca006132 commented Oct 15, 2024

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.

elalish
elalish reviewed Oct 15, 2024
View reviewed changes
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.

@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.

@pca006132 pca006132 merged commit e718288 into master Oct 18, 2024
19 checks passed
@pca006132 pca006132 deleted the fix-python branch October 18, 2024 17:07
@elalish elalish mentioned this pull request Nov 5, 2024
Merged
@pbsds pbsds mentioned this pull request Dec 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elalish elalish elalish approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
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
@pca006132 @elalish
0