Skip to content
This repository was archived by the owner on Jun 10, 2020. It is now read-only.

ENH: Add type annotations for the np.core.fromnumeric module: part 1/4 #67

Merged
merged 8 commits into from
Apr 24, 2020
Merged

ENH: Add type annotations for the np.core.fromnumeric module: part 1/4 #67

merged 8 commits into from
Apr 24, 2020

Conversation

BvB93
Copy link
Member

@BvB93 BvB93 commented Apr 21, 2020

Continuation of #59.

Introduced type hints for the first 11 (public) functions from the np.core.fromnumeric module.

Functions

  • take
  • reshape
  • choose
  • repeat
  • put
  • swapaxes
  • transpose
  • partition
  • argpartition
  • sort
  • argsort

@BvB93
Copy link
Member Author

BvB93 commented Apr 21, 2020

It seems there are some minor differences between the formatting style of black 18.9b0 and the later 19.10b0 version, to the point were formatting with the latter will fail the tests (as 18.9b0 is pinned in test-requirements.txt)

Copy link
Member

@person142 person142 left a comment

Choose a reason for hiding this comment

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

Haven't had time to look through everything carefully, but had a few initial comments.

@overload
def take(
a: _ArrayLike,
indices: _ArrayLikeInt,
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 _ArrayLikeInt will not end up being general enough because you can do things like

>>> x = np.arange(9).reshape(3, 3)
>>> np.take(x, [[0, 1], [1, 2]], axis=1)
array([[[0, 1],
        [1, 2]],

       [[3, 4],
        [4, 5]],

       [[6, 7],
        [7, 8]]])

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true, I forgot the Sequence[int] syntax does not support arbitrary levels of nesting (which is what we need here).
I don't have a real solution for this, surprisingly fundamental, problem besides adding Unions up to some arbitrary level of nesting that we can consider "good enough".

Copy link
Member

Choose a reason for hiding this comment

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

So far we've just been doing Sequence (i.e. Sequence[Any]). I think the long-term fix is MyPy et. al. supporting recursive types; until then we're stuck with being imprecise.

It's not the best, but I think it's what we have right now. (Though I have also considered the

adding Unions up to some arbitrary level of nesting

It felt too gross to do, but maybe there's more discussion to be had on it in an issue.)

Copy link
Member Author

Choose a reason for hiding this comment

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

It felt too gross to do, but maybe there's more discussion to be had on it in an issue.)

Gross it most definetly is; I've changed it to Sequence[Any] for now (837eec6).

@@ -0,0 +1,60 @@
import numpy as np
Copy link
Member

Choose a reason for hiding this comment

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

Maybe there's a better name than fromnumeric.py? Maybe not though. At some point we're going to need to organize things better, but it's not hard to shuffle stuff around later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently it's named after the corresponding module in NumPy (np.core.fromnumeric).
I'm all ears if you have suggestions, but I feel that the current name is already pretty decent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Im any case, I've added a module-level docstring to the tests for now to clarify that the relevant functions are from np.core.fromnumeric (37cbded).

Bas van Beek added 2 commits April 21, 2020 20:27
* Renamed the ``_Scalar`` TypeVar to ``_ScalarGeneric`` (#67 (comment))
* Expanded the scope of ``_ArrayLikeInt`` up to sextuple levels of Sequence nesting (#67 (comment) and #67 (comment))
Copy link
Member

@person142 person142 left a comment

Choose a reason for hiding this comment

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

Had some more comments-also could you remove the TODOs? Basically everything in the stubs needs reworking once ndarray is generic, so I don't think we should note that in individual places.

Anyway, sorry this review is taking so long... these functions really are quite a beast to type aren't they?

axis: Optional[int] = ...,
out: Optional[ndarray] = ...,
mode: _Mode = ...,
) -> generic: ...
Copy link
Member

Choose a reason for hiding this comment

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

This isn't always true I don't think:

>>> type(np.take(datetime.timedelta(days=1), 0))
<class 'datetime.timedelta'>
>>> type(np.take(datetime.date.today(), 0))
<class 'datetime.date'>

Works for the others though. There might be diminishing returns getting this to work until such a time as we have a templating system in place; then it can generate specific overloads like bool -> np.bool_ etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

If thus is only exception to the rule, then I feel it might be worthwhile to swap generic with an union for the time being.

* The ``order`` parameter cannot be an ndarray, it has to be proper Sequence; change its annotation to ``Union[None, str, Sequence[str]]`` (see #67 (comment), #67 (comment) and #67 (comment))
* ``datetime.datetime`` and ``datetime.timedelta`` are also valid return types besides ``np.generic`` and ``np.ndarray`` (see #67 (comment))
* Removed all ``"TODO"`` comments expect for ``_ArrayLikeIntNested`` (#67 (review))
@person142 person142 merged commit 2011153 into numpy:master Apr 24, 2020
@person142
Copy link
Member

Thanks @BvB93!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants