-
-
Notifications
You must be signed in to change notification settings - Fork 31
Add array conversion methods to ndarray #21
Conversation
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.
Thanks for these PRs! I'll just point out that we also have tests setup that might be worth augmenting for any complex cases: https://github.com/numpy/numpy-stubs/tree/master/tests
numpy-stubs/__init__.pyi
Outdated
def copy(self, order: str=...) -> ndarray: ... | ||
def view(self, | ||
dtype: Union[_DtypeLike, Type]=..., | ||
type: Any=...) -> ndarray: ... |
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.
This should probably substitute Type
-> Type[ndarray]
and Any
-> Type[ndarray]
.
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.
Thank you! Fixed. Covered with 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.
This is great, but just to be clear you don't need tests for everything -- especially not for simple methods like dumps()
that just returns bytes
.
I would save tests for complex cases where it's not immediately evident that the code is correct, e.g., where you have overloads or type variables.
tests/reveal/ndarray_conversion.py
Outdated
reveal_type(nd.view(np.int64)) # E: numpy.ndarray | ||
reveal_type(nd.view(dtype=np.int64)) # E: numpy.ndarray | ||
reveal_type(nd.view(np.int64, np.matrix)) # E: numpy.ndarray | ||
reveal_type(nd.view(type=np.matrix)) # E: numpy.ndarray |
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.
In theory, I think this should be numpy.matrix
.
Or we can could make a dummy subclass, if np.matrix
isn't defined, e.g.,
class SubArray(np.ndarray):
pass
reveal_type(nd.view(type=SubArray)) # E: SubArray
Does that actually 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.
Thank you for review! I've removed simple tests and fixed type hints for view
.
Remove simple ones and leave comments for consistency
Its stub was splitted into all possible overloads. `view(self, dtype: Type[_NdArraySubClass], type: Type[_NdArraySubClass])` could not be used.
Actual types are like: