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

feat(types) Numpy.typing.NDArray #5212

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

InvincibleRMC
Copy link
Contributor

@InvincibleRMC InvincibleRMC commented Jun 29, 2024

Description

Switches to using numpy.typing.NDArray for typing annotations over numpy.ndarray. This is because numpy.ndarray is implemented with the first argument for some future size annotation. See numpy/numpy#16544. Since the first argument is for the size annotation the typing of the numpy.ndarray is basically not being used. I also modified the eigen matrix/tensor to numpy.typing.NDArray as well even though stubgen fails to generate stubs since numpy.typing.NDArray[numpy.float64[...]] is not a valid type.

def foo(x: numpy.typing.NDArray[numpy.float64]) -> None:
    reveal_type(x)  # ndarray[Any, dtype[floating[_64Bit]]
    reveal_type(x.dtype)  # dtype[float64]

def bar(x: numpy.ndarray[numpy.float64]) -> None:
    reveal_type(x)  # ndarray[float64, Any]
    reveal_type(x.dtype)  # Any

Suggested changelog entry:

    Switched to `numpy.typing.NDArray` 

@rwgk
Copy link
Collaborator

rwgk commented Jun 30, 2024

I don't know enough about typing to meaningfully weigh the pros and cons of changing from ndarray to NDArray. @henryiii could you help with that?

Regarding the tuple[()] change, we have this already:

template <>
struct handle_type_name<typing::Tuple<>> {
// PEP 484 specifies this syntax for an empty tuple
static constexpr auto name = const_name("tuple[()]");
};

Is your change needed anyway? Could you send a separate, small PR for that, including a test that fails without your change?

@InvincibleRMC InvincibleRMC changed the title feat(types) Numpy.typing.NDArray and empty tuple feat(types) Numpy.typing.NDArray Jun 30, 2024
@henryiii
Copy link
Collaborator

NDArray is the public type for NDArrays, and hides the "size" related type (as described above). It also uses a different form for the DType. Just curious, what about changing it to numpy.ndarray[Any, numpy.float64]?

FYI, I get an error on your example:

$ bat tmp.py
───────┬─────────────────────────────────────────
       │ File: tmp.py
───────┼─────────────────────────────────────────
   1   │ import numpy
   2   │ from typing import Any
   3   │
   4   │ v: numpy.typing.NDArray[numpy.float64]
   5   │ reveal_type(v)
   6   │ reveal_type(v.dtype)
   7   │
   8   │ q: numpy.ndarray[Any, numpy.float64]
   9   │ reveal_type(q)
  10   │ reveal_type(q.dtype)
  11   │
  12   │
───────┴─────────────────────────────────────────
$ pipx install mypy
$ pipx inject mypy numpy
$ mypy tmp.py
tmp.py:5: note: Revealed type is "numpy.ndarray[Any, numpy.dtype[numpy.floating[numpy._typing._64Bit]]]"
tmp.py:6: note: Revealed type is "numpy.dtype[numpy.floating[numpy._typing._64Bit]]"
tmp.py:8: error: Type argument "floating[_64Bit]" of "ndarray" must be a subtype of "dtype[Any]"  [type-var]
tmp.py:9: note: Revealed type is "numpy.ndarray[Any, numpy.floating[numpy._typing._64Bit]]"
tmp.py:10: note: Revealed type is "numpy.floating[numpy._typing._64Bit]"
Found 1 error in 1 file (checked 1 source file)

@InvincibleRMC
Copy link
Contributor Author

InvincibleRMC commented Aug 12, 2024

In my example I have numpy.ndarray[numpy.float64] which is what was currently being done to shown the error. You have numpy.ndarray[Any, numpy.float64] which is also incorrect. It would need to be numpy.ndarray[Any, numpy.dtype[numpy.float64]]] to create the proper type annotation without numpy.typing. If you would prefer numpy.ndarray[Any, numpy.dtype[numpy.float64]]] I can update it to be that.

@gentlegiantJGC
Copy link
Contributor

The valid options are

  1. numpy.ndarray[Any, numpy.dtype[numpy.float64]]]
  2. numpy.typing.NDArray[numpy.float64]

I think numpy.typing.NDArray is the cleanest way to resolve this issue.

I can't find any documentation for the shape, writeable and contiguous attributes in the type hint. Is this correct?
numpy.typing.NDArray[numpy.float32[5, 6], flags.writeable, flags.f_contiguous]
I don't think these are valid for numpy.ndarray either.

@henryiii
Copy link
Collaborator

Needs update after #5486

@InvincibleRMC
Copy link
Contributor Author

@timohl I have tried some various things like to get io_name working with templated arguments. Is there something obvious I am missing?

    PYBIND11_TYPE_CASTER(type,
                         io_name("numpy.typing.ArrayLike",
                                 "numpy.typing.NDArray["
                                     + npy_format_descriptor<T>::name + "]"));

@timohl
Copy link
Contributor

timohl commented Jan 25, 2025

@timohl I have tried some various things like to get io_name working with templated arguments. Is there something obvious I am missing?

Yes, but maybe not so obvious.
io_name only supports constexpr c-string arguments.
The type of npy_format_descriptor<T>::name is some instance of the template struct descr.
This disallows nesting of io_name which would break things.
Maybe something like this could work:npy_format_descriptor<T>::name.text

I have drafted a working version using io_name with c-strings here: #5502.
With this, I was able to build Open3D (with minor modification), run pybind-stubgen, and run mypy successfully on all example code without errors about numpy types from Eigen. 🚀

We should discuss what kind of numpy type we should use.
In #5502 I used something like this:
For args: typing.Annotated[numpy.typing.ArrayLike, numpy.float32, "[m, 1]", "flags.writeable", "flags.c_contiguous"]
For return: typing.Annotated[numpy.typing.NDArray[numpy.float32], "[m, 1]", "flags.writeable", "flags.c_contiguous"]

I think a combination of typing.Annotated, numpy.typing.ArrayLike and numpy.typing.NDArray would be best to keep all available information while providing valid types.

Open questions:

  • Format of typing.Annotated arguments: I used strings to use "[m, 1]" as shape, since m is usually not defined. Other options might be to use something like nptyping or annotated-types but adding 3rd party dependencies should rather be an option in stubgen, not pybind itself in my opinion
  • Should all flags be shown in return type aswell or only in args? I think they are used to provide better error messages when a user passes an array that does not conform
  • Where to use ArrayLike or NDArray? Right now, I just added it to all args, but I have not completely checked the code of type casters for every Eigen variant if they really accept for example lists as input.

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.

5 participants