Python more numpy #656
Python more numpy #656
Conversation
a927c23 to
d5444ad
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #656 +/- ##
==========================================
- Coverage 91.66% 91.64% -0.03%
==========================================
Files 37 36 -1
Lines 4737 4737
==========================================
- Hits 4342 4341 -1
- Misses 395 396 +1 ☔ View full report in Codecov by Sentry. |
d5444ad to
311cbc7
Compare
|
There is a caster for Not sure if arguments accepting numpy arrays can accept normal python list. |
|
I think there are also some other tiny fixes that can be added into this PR. Missing manifold/bindings/python/manifold3d.cpp Lines 368 to 375 in eba866d This should be an int instead of a double: manifold/bindings/python/manifold3d.cpp Line 997 in eba866d |
A quick test suggests the answer is no. I guess the auto casting approach has the best upside. I'll tinker with it some more. |
7d97aea to
d679750
Compare
|
I removed almost all of the lambda functions in the bindings. This is enabled by improved auto casting of glm::X, std::vectorglm::X and VecViewglm::X. A few remain for overload-resolution, dealing with stray types like |
d679750 to
f3fcdc0
Compare
|
I also removed some overloads like |
|
Thanks for the cleanup. I think much of the code comes from docstrings, it would be nice if we can find a way to unify the documentation for the main C++ library and the 3 different bindings, but I guess this is a hard problem (e.g. name inconsistencies, specifics for certain binding). |
|
This looks awesome, thanks! I always appreciate cleanups that will ease maintenance going forward. I'll review more closely after the other PR is merged and this comes out of draft. |
Also add CrossSection::Bounds()
f3fcdc0 to
0895569
Compare
|
I think you probably need to push a dummy commit to trigger the CI build. We should fix this CI problem later... |
| "of the same shape and type in return. The input array can be " | ||
| "modified and returned if desired. " | ||
| "\n\n" | ||
| ":param f: A function that modifies multiple vertex positions.") |
There was a problem hiding this comment.
thinking about it, we may want documentation about the type signature of the function. same for warp above and set_properties below.
| @@ -568,28 +530,24 @@ NB_MODULE(manifold3d, m) { | |||
| "as empty. Likewise, empty meshes may still show NoError, for " | |||
| "instance if they are small enough relative to their precision to " | |||
| "be collapsed to nothing.") | |||
| .def_prop_ro( | |||
| .def( | |||
There was a problem hiding this comment.
we should probably state that this is a breaking change, i.e. property vs method.
|
What do you think about |
|
I think every methods on |
There was a problem hiding this comment.
That's a lot of great clean-up. LGTM; I'll leave it to @pca006132 for final approval since I'm no Python master.
| c = c.warp_batch(lambda ps: ps * [1, 0.5] + [1, 0]) | ||
|
|
||
|
|
||
| def all_manifold(): |
There was a problem hiding this comment.
Thanks - this big basic test file is a great idea. Nice as an API index too.
|
Looks like a pretty simple error the python test caught, but are we only running Python tests on some of our builds? Is that intentional @pca006132? |
|
Forgot if this is intentional or not, we can just enable it for every build. We can also enable all tests even for draft PR, so we will not get skipped tests when we change the state from draft to ready for review. I think I will wait for @wrongbad to signal this ready for review, not sure if he is done with it now. |
930b91b to
458454e
Compare
458454e to
a4b0e74
Compare
|
At this point I'm just hoping to get all the tests passing, and resolve any review concerns. I'll look at generating doc-strings in a follow-up. |
|
I will have a look at it tmr. We can leave the documentation generation thing later. I feel that it should not be too complicated, but may require parsing the documentation to get a list of parameters for substitution. Can probably be done together with the python stub. |
| } | ||
| } | ||
| } else { | ||
| int num_vec = PyObject_Size(src.ptr()); // negative on failure |
There was a problem hiding this comment.
This is surprising. Just curious, any reference on this?
There was a problem hiding this comment.
So the caster function this is inside is noexcept and if you call nb::len on a float it throws, and terminates the program. So I looked into source for nb::len and found PyObject_Size which gives a softer fallback mode when user calls a func with the wrong type, we return false and nanobind generates a useful error.
|
So I guess this can be merged? |
* [python] numpy updates and cleanup Also add CrossSection::Bounds() * cleanup - const and reference args in py bind * fix unnamed args * all_apis.py more coverage * fixup - more param renaming, docstring fixes * fixup - run black * fixup - move const to left side * fixup extrude.py get_volume * fix python examples for api update
Note: this branch is on top of WarpBatch commit - that will disappear when that other PR merges.
Changed
hull_pointsto accept numpy point arrays. Also removed several redundant numpy shape checks - these shape constraints were already enforced by the shape template parameters.sdf/level_setcould benefit from numpy batching but requires deeper refactoring (#598)set_propertiesalso could benefit, and I'd like to look into it more here.That's all that I see. Let me know if I missed anything.
While I was auditing the API, I noticed something else needing cleanup. Many functions need a single vector/point, but the convention is all over the map. Some take 3 floats, some take a tuple, some take a
glm::vec3, some take a numpy(3), and several have overloads to support some random subset of those. We should probably converge on 1 way. My preference would be tuple, but I'm curious to hear other opinions.