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

Eigen: fix overloaded call to make_tuple is ambiguous #414

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

hmenke
Copy link
Contributor

@hmenke hmenke commented Feb 1, 2024

Without fully qualifying nanobind::make_tuple the new test raises the following compiler error.

In file included from /home/henri/Code/nanobind/tests/test_eigen.cpp:3:
/home/henri/Code/nanobind/include/nanobind/eigen/sparse.h: In instantiation of ‘static nanobind::handle nanobind::detail::type_caster<T, typename std::enable_if<is_eigen_sparse_matrix_v<T>, int>::type>::from_cpp(const T&, nanobind::rv_policy, nanobind::detail::cleanup_list*) [with T = Eigen::SparseMatrix<std::complex<double> >]’:
/home/henri/Code/nanobind/include/nanobind/eigen/sparse.h:99:24:   required from ‘static nanobind::handle nanobind::detail::type_caster<T, typename std::enable_if<is_eigen_sparse_matrix_v<T>, int>::type>::from_cpp(T&&, nanobind::rv_policy, nanobind::detail::cleanup_list*) [with T = Eigen::SparseMatrix<std::complex<double> >]’
/home/henri/Code/nanobind/include/nanobind/nb_func.h:153:40:   required from ‘PyObject* nanobind::detail::func_create(Func&&, Return (*)(Args ...), std::index_sequence<Is2 ...>, const Extra& ...) [with bool ReturnRef = false; bool CheckGuard = true; Func = nanobind_init_test_eigen_ext(nanobind::module_&)::<lambda()>; Return = Eigen::SparseMatrix<std::complex<double> >; Args = {}; long unsigned int ...Is = {}; Extra = {nanobind::scope, nanobind::name}; PyObject = _object; std::index_sequence<Is2 ...> = std::integer_sequence<long unsigned int>]’
/home/henri/Code/nanobind/include/nanobind/nb_func.h:207:37:   required from ‘void nanobind::cpp_function_def(Func&&, const Extra& ...) [with Func = nanobind_init_test_eigen_ext(nanobind::module_&)::<lambda()>; Extra = {scope, name}; typename std::enable_if<is_lambda_v<typename std::remove_reference<_Tp>::type>, int>::type <anonymous> = 0]’
/home/henri/Code/nanobind/include/nanobind/nb_func.h:256:21:   required from ‘nanobind::module_& nanobind::module_::def(const char*, Func&&, const Extra& ...) [with Func = nanobind_init_test_eigen_ext(nanobind::module_&)::<lambda()>; Extra = {}]’
/home/henri/Code/nanobind/tests/test_eigen.cpp:169:10:   required from here
/home/henri/Code/nanobind/include/nanobind/eigen/sparse.h:134:42: error: call of overloaded ‘make_tuple(std::remove_reference<nanobind::ndarray<nanobind::numpy, std::complex<double>, nanobind::shape<18446744073709551615> >&>::type, std::remove_reference<nanobind::ndarray<nanobind::numpy, int, nanobind::shape<18446744073709551615> >&>::type, std::remove_reference<nanobind::ndarray<nanobind::numpy, int, nanobind::shape<18446744073709551615> >&>::type)’ is ambiguous
  134 |             return matrix_type(make_tuple(
      |                                ~~~~~~~~~~^
  135 |                                    std::move(data), std::move(inner_indices), std::move(outer_indices)),
      |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/henri/Code/nanobind/include/nanobind/nanobind.h:49,
                 from /home/henri/Code/nanobind/include/nanobind/stl/complex.h:12,
                 from /home/henri/Code/nanobind/tests/test_eigen.cpp:1:
/home/henri/Code/nanobind/include/nanobind/nb_cast.h:455:7: note: candidate: ‘nanobind::tuple nanobind::make_tuple(Args&& ...) [with rv_policy policy = nanobind::rv_policy::automatic; Args = {ndarray<numpy, std::complex<double>, shape<18446744073709551615> >, ndarray<numpy, int, shape<18446744073709551615> >, ndarray<numpy, int, shape<18446744073709551615> >}]’
  455 | tuple make_tuple(Args &&...args) {
      |       ^~~~~~~~~~
In file included from /nix/store/f94yr35af3xdiscbj6cp6kafvmn55gv9-gcc-12.3.0/include/c++/12.3.0/functional:54,
                 from /nix/store/wfcm6p8wh1lnk2iwcvyx0fkg9sjqrq72-eigen-3.4.0/include/eigen3/Eigen/Core:85,
                 from /home/henri/Code/nanobind/include/nanobind/eigen/dense.h:14,
                 from /home/henri/Code/nanobind/tests/test_eigen.cpp:2:
/nix/store/f94yr35af3xdiscbj6cp6kafvmn55gv9-gcc-12.3.0/include/c++/12.3.0/tuple:1577:5: note: candidate: ‘constexpr std::tuple<typename std::__strip_reference_wrapper<typename std::decay<Ts>::type>::__type ...> std::make_tuple(_Elements&& ...) [with _Elements = {nanobind::ndarray<nanobind::numpy, complex<double>, nanobind::shape<18446744073709551615> >, nanobind::ndarray<nanobind::numpy, int, nanobind::shape<18446744073709551615> >, nanobind::ndarray<nanobind::numpy, int, nanobind::shape<18446744073709551615> >}]’
 1577 |     make_tuple(_Elements&&... __args)
      |     ^~~~~~~~~~

@wjakob
Copy link
Owner

wjakob commented Feb 1, 2024

I don't know of a better way, and this PR seems quite reasonable. Could you amend it to not have the FIXME comment?

@hmenke
Copy link
Contributor Author

hmenke commented Feb 1, 2024

Thank you for quick assessment. I've addressed your comment.

@wjakob
Copy link
Owner

wjakob commented Feb 1, 2024

One small request (since I haven't run into this compilation issue on my end). Does also work if you import nanobind's make_tuple function into the detail namespace?

I.e., add `using nanobind::make_tuple;`` at line 23 of this file after the namespace opens. I don't think you need to change your PR even if that fixes the issue. I'm just really confused about why C++ complains here and wonder if that would also resolve the issue.

@hmenke
Copy link
Contributor Author

hmenke commented Feb 1, 2024

No, using nanobind::make_tuple does not fix the error, but I also don't see why it should. The using declaration does not signify any kind of preference in lookup but rather enables ADL where it would normally not be considered (the classic case being using std::swap).

One interesting observation is maybe that it breaks with std::complex<double>

m.def("sparse_complex", []() -> Eigen::SparseMatrix<std::complex<double>> { return {}; }); // bad :(

but works with double

m.def("sparse_real", []() -> Eigen::SparseMatrix<double> { return {}; }); // good :)

@wjakob
Copy link
Owner

wjakob commented Feb 1, 2024

Pretty weird. This has made me realize that I don't know how ADL works. Somehow std::complex activates std namespace related functions, which does not happen when passing a double.

@wjakob wjakob merged commit 72831bb into wjakob:master Feb 1, 2024
24 checks passed
@hmenke hmenke deleted the eigen branch February 1, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants