MET-30: Implement Geometry Primitives - #12
Conversation
d4c3430 to
17ff36c
Compare
horizon-blue
left a comment
horizon-blue
left a comment
There was a problem hiding this comment.
LGTM! Feel free to merge whenever you feel ready :)
| from scipy.spatial.transform import Rotation as Rot | ||
|
|
||
| from genmetaballs import _genmetaballs_bindings as _gmbb | ||
| from genmetaballs._genmetaballs_bindings import geometry as geometry |
There was a problem hiding this comment.
How do you feel about exposing the geometry module in genmetaballs.core similar to the other binding modules?
There was a problem hiding this comment.
I forgot to do this!
| .def_ro("x", &Vec3D::x) | ||
| .def_ro("y", &Vec3D::y) | ||
| .def_ro("z", &Vec3D::z) | ||
| .def("__add__", static_cast<Vec3D (*)(const Vec3D, const Vec3D)>(&operator+)) |
There was a problem hiding this comment.
Dumb question: is there a reason that we're preferring the explicit type casting rather than using the nb::self + nb::self shorthands? 👀
There was a problem hiding this comment.
Not really, I was looking at the nanobind docs and it seemed like the shorthand creates an anonymous function which calls the add. Technically, this could create one extra call frame (calling the binding calls an anonymous function that calls operator+, but here the binding directly calls operator+). I think the performance will be negligible. Will switch to this pattern.
| Rotation rot_; | ||
| Vec3D tran_; | ||
|
|
||
| CUDA_CALLABLE Pose(const Rotation rot, const Vec3D tran) : rot_{rot}, tran_{tran} {} |
There was a problem hiding this comment.
this is really minor but I wonder if we can accept references here? 🤔
There was a problem hiding this comment.
Isn't this like 7 floats? Would a reference make a difference?
There was a problem hiding this comment.
Probably not by much tbh, but I guess it wouldn't hurt either? ;)
| -readability-avoid-const-params-in-decls, | ||
| -readability-braces-around-statements, | ||
| -readability-isolate-declaration, | ||
| -readability-math-missing-parentheses, |
There was a problem hiding this comment.
I bet this linting is getting annoying haha, i'm also always adding suppressions here just to get over the dumb linting checks. Good addition!
arijit-dasgupta
left a comment
arijit-dasgupta
left a comment
There was a problem hiding this comment.
LGTM! just requesting that __rmul__ fix
| .def("__sub__", static_cast<Vec3D (*)(const Vec3D, const Vec3D)>(&operator-)) | ||
| .def("__neg__", static_cast<Vec3D (*)(const Vec3D)>(&operator-)) | ||
| .def("__mul__", static_cast<Vec3D (*)(const Vec3D, float)>(&operator*)) | ||
| .def("__rmul__", static_cast<Vec3D (*)(float, const Vec3D)>(&operator*)) |
There was a problem hiding this comment.
Great catch on the __rmul__, it was subtle to me, however I think this binding is incorrect. I checked and Python expects it to be in the same order as __mul__. I'll make a suggestion here that works, but i'd also suggest we add tests for these methods inside of test_vec3d_ops from test_geometry.py.
| .def("__rmul__", static_cast<Vec3D (*)(float, const Vec3D)>(&operator*)) | |
| .def("__rmul__", static_cast<Vec3D (*)(const Vec3D, float)>(&operator*)) |
There was a problem hiding this comment.
Great catch to you sir! Catch of the day award! I used the nanobind idiom as @horizon-blue suggested so this should be fixed now too!
|
Actually, nvm, i'll approve so that you can merge asap after fixing the |
|
@arijit-dasgupta @horizon-blue I've made the following modifications based on your suggestions:
|
3 participants
This PR adds the geometry we scoped during our 11/16/2025 and 11/18/2025 meetings. Specifically, the PR provides:
Vec3Dtype for 3D vector geometry;Rotationtype representing 3D rotations;Posetype representing 3D rigid transformations.Bindings and correctness fuzz tests against
scipyare available inbindings.cuandtest_geometry.py, respectively. To run the tests simply runpixi run test.