cast<Eigen::Ref...>(handle) and conversions

[WKarel]

type_caster<Eigen::Ref...> owns a type_caster<Eigen::Map...>, which owns a type_caster<ndarray...>.

type_caster<Eigen::Ref...>::from_python calls its type_caster<Eigen::Map...>, which in turn calls its type_caster<ndarray...>, which calls ndarray_import to do its job.

If ndarray_import gets a dltensor of matching properties, it creates a view into the memory of the Python instance.
Otherwise (e.g. if the dtype does not match), it copy-converts the data if called with convert=true, or it returns nullptr, making from_python return false.
The latter works if Eigen::Ref<...> is used as function argument, because the type_caster is kept alive until the bound function returns, and so the memory owned by the temporary ndarray is valid during the bound function call.

When
static constexpr bool IsClass = false;
is inserted into type_caster<Eigen::Ref...>, then cast<Eigen::Ref...>(handle) compiles. However, the type_caster then is a temporary within cast, which is destroyed upon returning from cast. Hence, if ndarray_import does a conversion, the result of cast points to invalid memory.

1. Are type_caster<Eigen::Ref...> and type_caster<Eigen::Map...> really supposed to not define bool IsClass? Then cast<Eigen::Ref...> and cast<Eigen::Map...> do not compile, which is a pity.
2. Or are they supposed to define it? Then type_caster<Eigen::Map...>::from_python would need to disallow conversions by type_caster<ndarray...>::from_python. This would affect the treatment of bound function arguments, too. Even better I would find more fine-grained type_caster specializations for Eigen::Ref. While Eigen::Ref<const T...> maps data as well, it can also own that data, meaning that it could own the result of conversions, as is the case in pybind11.

Also, there are quite obvious flaws in type_caster<Eigen::Map>::strides():

1. it accesses caster.value.stride(1) even if the ndarray has less than 2 dimensions.
2. StrideType::InnerStrideAtCompileTime and StrideType::OuterStrideAtCompileTime can be 0, meaning contiguous memory to Eigen. In this common case (e.g. default of Eigen::Map), StrideType must not be initialized with 0s, but with the respective stride values for contiguous memory, or be default-constructed, to avoid assertions failing in Eigen.

Concerning the type_caster specializations for Eigen types in general, they specialize for all scalar types, even though they handle only those that can be handled by dlpack (static_assert in dtype<T>(). I would advocate for SFINAE-limiting them to the scalar types they can actually handle, so users may provide specializations for their custom types.

Would a pull request be welcome?

Demonstration code:
C++:

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

namespace nb = nanobind;

NB_MODULE(nanobind_eigen, m)
{
  m.def("RefVectorXd", [](Eigen::Ref<Eigen::MatrixXd> arg) -> Eigen::MatrixXd
    {
      return arg;
    });

  m.def("SequenceOfRefVectorXd_castNoConvert", [](nb::sequence seq) -> Eigen::MatrixXd
    {
      return nb::cast<Eigen::Ref<Eigen::MatrixXd>>(seq[0], false);
    });

  m.def("SequenceOfRefVectorXd_cast", [](nb::sequence seq) -> Eigen::MatrixXd
    {
      // If seq[0] provides a dltensor with non-matching properties, then the EigenRef points at invalid memory.
      // Copy-conversion of Eigen::MatrixXd then crashes at some point.
      return nb::cast<Eigen::Ref<Eigen::MatrixXd>>(seq[0]);
    });

}

Python:

import nanobind_eigen
import numpy as np

mat = np.ones((1000, 500), dtype=int, order='F')

print('RefVectorXd... ', end='', flush=True)
mat2 = nanobind_eigen.RefVectorXd(mat)
print('done', flush=True)
np.testing.assert_array_equal(mat2, 1.)

print('SequenceOfRefVectorXdNoConvert... ', end='', flush=True)
try:
    nanobind_eigen.SequenceOfRefVectorXd_castNoConvert([mat])
except RuntimeError:
    print('done', flush=True)
else:
    raise Exception('RuntimeError expected')
    
print('SequenceOfRefVectorXd... ', end='', flush=True)
mat2 = nanobind_eigen.SequenceOfRefVectorXd_cast([mat])
# Not reached due to crash somewhere on the line above:
print('done', flush=True)
np.testing.assert_array_equal(mat2, 1.)

[wjakob]

Dear @WKarel,

these sound like pretty severe flaws. Yes, a PR with fixes would be more than welcomed. You could

1. Add IsClass=false members, those are an accidental omission.
2. The right thing will be to prevent conversion to references when a copy/conversion is needed, when the user calls nb::cast. The function argument conversion case is likely different because we have a cleanup-stack (cleanup argument) for temporaries that guarantees their existence until the call has successfully concluded, and that exists precisely to deal with such implicit conversions.
3. The strides issue sounds problematic. I recently fixed an assertion failure (in debug mode) for a related issue (a965f5b), but there might be more.

I don't really use Eigen myself and just created the caster for the convenience of others. But this also means that it's not that well tested with regards to all of the various corner cases. Help is definitely welcome.

[WKarel]

Dear @wjakob,

great, then I'll try to come up with a PR these days.

Cheers,
Willi.

[WKarel]

See #215