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.

Sign up for GitHub

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

Jump to bottom

[TYP] return array dtype is always unkown but it's actually specified as parameter #3602

Merged
merged 7 commits into from Apr 25, 2023
Merged

[TYP] return array dtype is always unkown but it's actually specified as parameter #3602

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
55b9c72
[TYP] return array dtype is always unkown but it's specified as param…
Mar 27, 2023
7eae4f2
[TYP] int32 is not always the default
Mar 28, 2023
f33ae9c
[TYP] strategy parameters type are related to each other
Mar 28, 2023
0ba72e8
authors, contribution & typo release.rst
Mar 28, 2023
12b9c9a
[TYP] dtype can actually be a DTypeLike or a SearchStrategy[DTypeLike]
Mar 28, 2023
1713b31
[TYP] strategy dtypes can be different, so long as casting is lossles…
Mar 28, 2023
1c7cd4d
Fix lint issues
Zac-HD Apr 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 9 additions & 5 deletions hypothesis-python/src/hypothesis/extra/numpy.py
View file
Open in desktop
Expand Up @@ -9,7 +9,7 @@
# obtain one at https://mozilla.org/MPL/2.0/.

import math
from typing import Any, Mapping, Optional, Sequence, Tuple, Union
from typing import Any, Mapping, Optional, Sequence, Tuple, TypeVar, Union

import numpy as np

Expand Down Expand Up @@ -37,6 +37,7 @@
from hypothesis.strategies._internal.numbers import Real
from hypothesis.strategies._internal.strategies import T, check_strategy
from hypothesis.strategies._internal.utils import defines_strategy
from numpy.typing import DTypeLike, NDArray
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might increase our minimum supported numpy version. That's fine if we have to, but I think we could probably use an if TYPE_CHECKING: block and string annotations to preserve compatibility?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I only checked for py37 compatibility.

NDArray was introduced in numpy 1.21 and DTypeLike in v1.20, v1.21 is from june 2022, I belive that's old enough,
I would personally introduce a install_requires=['numpy>=1.21'] in the setup.py, but the call is totally yours.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's as old as NEP-29 suggests supporting, which seems a quite principled way to make the decision. @honno do you have thoughts on this?

Regardless we should set our actual minimum version in extras_require - I doubt we actually still support back to 1.9.0 - and think about setting up a minimum-numpy-version CI job. The CI job should be a separate PR though!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's as old as NEP-29 suggests supporting, which seems a quite principled way to make the decision. @honno do you have thoughts on this?

I missed that, then we are good to go, no need for if TYPE_CHECKING right?


__all__ = [
"BroadcastableShapes",
Expand Down Expand Up @@ -374,15 +375,18 @@ def fill_for(elements, unique, fill, name=""):
return fill


D = TypeVar("D", bound=DTypeLike)


@defines_strategy(force_reusable_values=True)
def arrays(
dtype: Any,
dtype: D,
Zac-HD marked this conversation as resolved.
Show resolved Hide resolved
shape: Union[int, st.SearchStrategy[int], Shape, st.SearchStrategy[Shape]],
*,
elements: Optional[Union[st.SearchStrategy, Mapping[str, Any]]] = None,
fill: Optional[st.SearchStrategy[Any]] = None,
Zac-HD marked this conversation as resolved.
Show resolved Hide resolved
unique: bool = False,
) -> st.SearchStrategy[np.ndarray]:
) -> st.SearchStrategy[NDArray[D]]:
Comment on lines -385 to +389
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also declare the shape here, for constant shapes... albeit by going from two to four overloads (value or strategy for each of two arguments).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can get away without overloads in this case, by defining another typevar for shape but honestly I would like to leave this point for now until we sort out the rest the comments you made in this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me, I'll be very happy to get in whatever improvement we have and write up an issue with the rest. 👍

r"""Returns a strategy for generating :class:`numpy:numpy.ndarray`\ s.

* ``dtype`` may be any valid input to :class:`~numpy:numpy.dtype`
Expand Down Expand Up @@ -900,8 +904,8 @@ def integer_array_indices(
shape: Shape,
*,
result_shape: st.SearchStrategy[Shape] = array_shapes(),
dtype: np.dtype = "int",
) -> st.SearchStrategy[Tuple[np.ndarray, ...]]:
dtype: D = np.int32,
Zac-HD marked this conversation as resolved.
Show resolved Hide resolved
) -> st.SearchStrategy[Tuple[NDArray[D], ...]]:
"""Return a search strategy for tuples of integer-arrays that, when used
to index into an array of shape ``shape``, given an array whose shape
was drawn from ``result_shape``.
Expand Down