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

Start a list of modules which require typing #8198

Merged
merged 4 commits into from
Sep 20, 2023

Conversation

max-sixty
Copy link
Collaborator

Notes inline. Just one module so far!

Notes inline. Just one module so far!
# pandas defines in terms of com (converting to alpha in the algo)
# so use its function to get a com and then convert to alpha

com = _get_center_of_mass(com, span, halflife, alpha)
return 1 / (1 + com)


def move_exp_nanmean(array, *, axis, alpha):
def move_exp_nanmean(array: np.ndarray, *, axis: int, alpha: float) -> np.ndarray:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really true that array can only be np.ndarray? cupy arrays are not allowed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hadn't thought of that — I think it is possible: https://docs.cupy.dev/en/stable/user_guide/interoperability.html#numba

Do you know what the correct item to use here is? ArrayLike?

Copy link
Contributor

Choose a reason for hiding this comment

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

In #8075 we are testing T_DuckArray = typing.TypeVar("T_DuckArray", bound=typing.Any) for self.data, which should imply "an array that follows the array api".

Nothing is in main at the moment though, probably because it would open so many cans of worms. :D

I think the important thing here though is that we just have to put a T_DuckArray in types.py and start to use it anywhere Variable/DataArray.data is used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK — what should do we do for this PR? This is just trying to start a pattern of allowing some strict modes — I hadn't really intended it to be breaking new ground on array types... :)

I could replace with Any if we would rather have something vague that slightly wrong...

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion, there's no point in strict mode if we're just going to use Any.
Add T_DuckArray in types.py and use it here, hopefully mypy won't complain too much.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If #8075 merges soon, I'm happy to use that — otherwise I really did just open this to start an template for expanding strict mode, I would strongly prefer to keep this PR focused & generally allow PRs to be small marginal improvements rather than midas-touching lots of code.

(and while we should indeed attempt to not use Any, it's still better than nothing, because a function with some annotations still works to check types internally)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added T_DuckArray!

@max-sixty max-sixty added the plan to merge Final call for comments label Sep 19, 2023
@max-sixty max-sixty merged commit 8c21376 into pydata:main Sep 20, 2023
28 checks passed
@max-sixty max-sixty deleted the mypy-strict branch September 20, 2023 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plan to merge Final call for comments topic-rolling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants