-
Notifications
You must be signed in to change notification settings - Fork 85
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
Stubs for Array, ArrayOrNone, and CArray #1682
Conversation
Looks like we need to skip the NumPy traits-stubs tests if either NumPy is not installed or NumPy is not sufficiently recent (e.g., 1.20 or later). |
This is ready for review. My fear was that the fact that |
More on this: I've now tested in a freshly-created Python 3.8 venv without NumPy installed, but with dev versions of traits and traits-stubs installed, and I'm not seeing any adverse effect on typing with either pyright / PyLance or mypy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good.
Some questions about default value types, whether we want to expose AbstractArray
, and if we want more tests.
|
||
class HasArrayTraits(HasTraits): | ||
spectrum = Array(shape=(None,), dtype=np.float64) | ||
maybe_image = ArrayOrNone(shape=(None, None, 3), dtype=np.float64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want a test for CArray
?
Similarly, it might be good to test the dimension ranges (eg. `maybe_rgba = Array(shape=(None, None, (3,4)), ...)
Also, is it worth testing for invalid trait parameters (eg. passing a default value of None
to Array
)?
Some of this may be overkill if we expect that things will Just Work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably test CArray
. I'm a bit grumpy with CArray
, because at this point it's actually identical in functionality to Array
, so I'm thinking that we might want to deprecate it.
I can flesh out the tests a bit more to test invalid parameters.
As an aside, I think at some point we're going to need a better framework for these tests; right now, there's a lot of interaction between tests that should be independent of one another.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eg. passing a default value of None to Array
That particular example is troublesome: as far as I can tell, in general there's no way to type hint a function parameter that has a default value of None
, but for which the intended API is that None
is never passed explicitly.
I've added a few more tests, including tests for bad declarations.
# all legal possibilities - only those that are common. | ||
_DTypeLike = Union[np.dtype[Any], Type[Any], str] | ||
|
||
class Array(_TraitType[_ArrayLike, _Array]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need AbstractArray
as well?
A quick search and I can't see any uses of it, but it is nominally public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AbstractArray
isn't exposed in traits.api
, so I'm happy leaving it out of the stubs for now.
self, | ||
dtype: Optional[_DTypeLike] = ..., | ||
shape: Optional[_Shape] = ..., | ||
value: Optional[_ArrayLike] = ..., |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this array-like, or do we expect an actual array here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Array-like: e.g., you can provide a list:
>>> from traits.api import HasTraits, Array
>>> class A(HasTraits):
... foo = Array(shape=(None,), value=[1, 2, 3])
...
>>> a = A()
>>> a.foo
array([1, 2, 3])
self, | ||
dtype: Optional[_DTypeLike] = ..., | ||
shape: Optional[_Shape] = ..., | ||
value: Optional[_ArrayLike] = ..., |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question: is this _Array
or _ArrayLike
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same answer. :-) _ArrayLike
is the right thing, I think.
PR #1682 introduced what should have been a check for numpy.typing, that ended up as a check for numpy.testing instead. This PR fixes that. Also, PR #1682 should have failed on Python 3.6 because of the above, since on Python 3.6 the latest version of NumPy available doesn't have type hints. It didn't fail because we weren't populating the package data with the new numpy_examples directory, so the numpy-based tests weren't run. That's fixed in #1709.
PR #1682 introduced what should have been a check for numpy.typing, that ended up as a check for numpy.testing instead. This PR fixes that. Also, PR #1682 should have failed on Python 3.6 because of the above, since on Python 3.6 the latest version of NumPy available doesn't have type hints. It didn't fail because we weren't populating the package data with the new numpy_examples directory, so the numpy-based tests weren't run. That's fixed in #1709.
PR #1682 introduced what should have been a check for numpy.typing, that ended up as a check for numpy.testing instead. This PR fixes that. Also, PR #1682 should have failed on Python 3.6 because of the above, since on Python 3.6 the latest version of NumPy available doesn't have type hints. It didn't fail because we weren't populating the package data with the new numpy_examples directory, so the numpy-based tests weren't run. That's fixed in #1709.
This PR adds type stubs for the
Array
,ArrayOrNone
andCArray
trait types.Work in progress because I still need to figure out what happens with both the tests and the stubs when NumPy is not installed in the environment.Fixes #1657.