-
Notifications
You must be signed in to change notification settings - Fork 63
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
Do we still need numpyeigen? #110
Comments
numpyeigen is doing codegen for the "matrix" of possible inputs, so that igl bindings work for {float,double} or {int32,int64} or {colmajor,rowmajor} If there's a cleaner way to do this using pure pybind11 we should switch to it. |
numpyeigen is also gluing us to a particular pybind release (via the One option is to help maintain numpyeigen and its forked pybind11. (probably the fastest route to fixing the short-term issue with numpy). Another would be to let go of the numpyeigen depedency and use pybind11 or nanobind directly. I'm testing out this route on alecjacobson/nanobind After some consultation and thinking, it's not purely a good thing that we support native execution all datatypes. In particular, float32. Libigl in C++ is almost entirely tested and used with double precision. Running libigl with single precision could make subtle bugs and tolerances appear. In many typical cases, users would be happier that single precision input was quietly copied to the precision libigl is tested on (double). Perhaps pybind11::EigenDRef or nb::DRef didn't exist when numpyeigen was first written, but it effortlessly handles the copyless rowmajor/colmajor (order="f"/"c") issue. Finally, even if a copy is needed (e.g., float → double) or (e.g., non-standard int → int64) it's just a memcpy and for most libigl functions won't be anywhere near the bottleneck. If a user badly wants python bindings with a non-default integer or numeric type, we can expose these for that user to compile their own bindings. |
This all makes sense and it's reasonable to upcast if it's going to prevent subtle precision bugs. EigenDRef did not exist back when this was written. FWIW, I have this PR open in pybind11 that would completely alleviate the need for the hacks branch (pybind/pybind11#4451). I haven't had time to work on it, but it's not a huge lift. |
Thanks, Francis. I'm sprinting ahead with this solution then. Thank you for all the original work on the bindings and for numpyeigen 🫡 it served us well! |
Seems like pybind11 can directly interface to Eigen types. https://pybind11.readthedocs.io/en/stable/advanced/cast/eigen.html
So is numpyeigen no longer necessary?
The text was updated successfully, but these errors were encountered: