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

Do we still need numpyeigen? #110

Open
alecjacobson opened this issue Oct 25, 2021 · 4 comments · May be fixed by #243
Open

Do we still need numpyeigen? #110

alecjacobson opened this issue Oct 25, 2021 · 4 comments · May be fixed by #243

Comments

@alecjacobson
Copy link
Contributor

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?

@alecjacobson
Copy link
Contributor Author

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.

@alecjacobson
Copy link
Contributor Author

alecjacobson commented Nov 7, 2024

numpyeigen is also gluing us to a particular pybind release (via the hacks branch). Currently that seems to be preventing us from using modern numpy. See fwilliams/numpyeigen#67

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.

@fwilliams
Copy link
Collaborator

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.

@alecjacobson
Copy link
Contributor Author

alecjacobson commented Nov 7, 2024

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants