wjakob / nanobind

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

[OTHER]: ndarray <-> Eigen type mismatch error would be easier to interpret if supplied argument types included the provided `shape` #502

Closed breathe closed 3 months ago

breathe commented 3 months ago

Type error reporting could be improved

Got a type error like this when using nanobind

TypeError                                 Traceback (most recent call last)
Cell In[25], line 1
----> 1 fitted_klobuchar_model = fit_klobuchar_native(
      2             initial_klobuchar_alphas, initial_klobuchar_betas, tec_map_vec, toe
      3         )

TypeError: fit_klobuchar_native(): incompatible function arguments. The following argument types are supported:
    1. fit_klobuchar_native(initial_klobuchar_alphas: numpy.ndarray[dtype=float64, shape=(*), order='C'], initial_klobuchar_betas: numpy.ndarray[dtype=float64, shape=(*), order='C'], tec_map_vec: numpy.ndarray[dtype=float64, shape=(*), order='C'], toe: float) -> numpy.ndarray[dtype=float64, shape=(*), order='C']

Invoked with types: ndarray, ndarray, ndarray, float

The actual problem is that the shape of one of the ndarray's supplied was incompatible. That _is_a type error -- but it would be much easier to diagnose if the supplied shapes were provided in the error message

eg -- in this case we were passing a matrix where a vector was expected -- so if the error reported this:

Invoked with types: ndarray(shape=(*)), ndarray(shape=(*)), ndarray(shape=(*,*)), float

Then it would've been obvious where the incompatible value was coming from

wjakob commented 3 months ago

I agree that this would be useful. However, this is also in conflict with keeping the error message generation code as simple as possible (this is a "nano" library, and so many kinds of bells and whistles are considered beyond the scope of the project).

breathe commented 3 months ago

ok but it's strange -- in order to detect this error at all the relevant information has to already have been detected by something -- kind of a shame that the reporting about the types of the supplied argument values is unable to include that relevant mismatched type factor ...

wjakob commented 3 months ago

You are welcome to propose a PR if this is very important to you. But I suspect that adding such an error message will seriously complicate the code, and that will make it a non-starter.