wjakob / nanobind

nanobind: tiny and efficient C++/Python bindings
BSD 3-Clause "New" or "Revised" License
2.14k stars 161 forks source link

[BUG]: cannot pass `1d matrix` (numpy shape (1,1)) across nanobind as `Eigen::Matrix<double,-1,-1>` #544

Closed breathe closed 1 month ago

breathe commented 2 months ago

Problem description

I get a type error when I try to pass a 1d ndarray to c++ function accepting Eigen::Matrix<double,-1,-1> (or Eigen::Array<double,-1-1>.

Behavior here seems unexpected to me -- I don't know if this might be too hard an issue to be fixable implementation wise ...

Thanks much!

Reproducible example code

c++

#include <nanobind/nanobind.h>
#include <nanobind/eigen/dense.h>

namespace nb = nanobind;

void processMatrix(const Eigen::Matrix<double, -1, -1> &mat)
{
    std::cout << "Received matrix: \n"
              << mat << std::endl;
}

NB_MODULE(test_native, m)
{
    m.def("process_matrix", &processMatrix, nb::arg("mat"));
}

python

    from test_native import process_matrix
    import numpy as np

    v = np.array([[1.0]])

    process_matrix(v)

    print("done")

Error

Exception has occurred: TypeError       (note: full exception trace is shown but execution is paused at: _run_module_as_main)
process_matrix(): incompatible function arguments. The following argument types are supported:
    1. process_matrix(mat: numpy.ndarray[dtype=float64, shape=(*, *), order='F']) -> None

Invoked with types: ndarray
ThoreWietzke commented 2 months ago

please change your C++ function argument to const Eigen::Ref<const Eigen::Matrix<double, -1, -1>>& mat and see if that solves your issue.

I suspect that the ordering of the array is the problem, as Eigen uses a column major format and Numpy a row major format. You can also change the layout in Eigen with Eigen::RowMajor.

wjakob commented 2 months ago

If that is the explanation, then I'm surprised that the Eigen type caster does not make an implicit copy (which is explicitly allowed in this case.) Maybe @WKarel knows?

breathe commented 2 months ago

please change your C++ function argument to const Eigen::Ref<const Eigen::Matrix<double, -1, -1>>& mat and see if that solves your issue.

Interesting -- that works -- thank you very much!

I had thought that the row major vs column major ordering was implicitly handled with a copy where required. This signature also works!

void processMatrix(const Eigen::Matrix<double, -1, -1, Eigen::RowMajor> &mat)
{
    std::cout << "Received matrix: \n"
              << mat << std::endl;
}

Oddly enough, the original signature (with RowMajor on numpy side and ColMajor on Eigen side) works if you pass at least an ndarray with shape (>=2, >=2)

WKarel commented 2 months ago

I have never really worked on the type_caster for plain Eigen types (but mostly on Ref and Map), and there is surely room for improvement - see also https://github.com/wjakob/nanobind/discussions/504 As this type_caster makes a copy anyway, it should be most liberal on what to accept as nb::ndarray.

wjakob commented 1 month ago

Thanks for the report. The issue was unrelated to the Eigen<->nanobind interface and is fixed in a76cfff860392cf279a157be2dd1966c3ccc88a4.