-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
Notes inline. Just one module so far!
xarray/core/rolling_exp.py
Outdated
# 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: |
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 it really true that array can only be np.ndarray? cupy arrays are not allowed?
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 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
?
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 #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.
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.
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...
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 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.
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.
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)
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.
Added T_DuckArray
!
f91b8ef
to
184aeee
Compare
Notes inline. Just one module so far!