-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
TYPING: pandas/core/window.py #27391
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
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.
Looks pretty good - just a few things. Thanks for taking a look at this!
pandas/core/window.py
Outdated
min_periods: Optional[int] = None, | ||
center: Optional[bool] = False, | ||
win_type: Optional[str] = None, | ||
axis: Union[str, int] = 0, |
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.
Can you put this in pandas._typing as Axis
and reference here?
cc @topper-123 as this might conflict with your other PR
pandas/core/window.py
Outdated
def _wrap_result(self, result, block=None, obj=None): | ||
def _wrap_result( | ||
self, result, block=None, obj=None | ||
) -> Union[ABCSeries, ABCDataFrame]: |
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.
Should use FrameOrSeries
from pandas._typing
@@ -63,19 +63,19 @@ class _Window(PandasObject, SelectionMixin): | |||
"axis", | |||
"on", | |||
"closed", | |||
] | |||
] # type: List[str] |
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.
Was this actually throwing an error? Not opposed to it just surprised Mypy wouldn't infer this already
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.
Don't think so; just thought to go ahead and type.
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.
lgtm - thanks for typing and simplifying signatures
Thanks for reviewing @WillAyd! |
* TYPING: pandas/core/window.py * Address comments * Remove unused import * Sort imports
values = values.copy() | ||
values[np.isinf(values)] = np.NaN | ||
# Always convert inf to nan | ||
values[np.isinf(values)] = np.NaN |
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.
You missed a copy call here. Results in kernc/backtesting.py#14 on write-protected arrays.
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
Includes simplifying some code paths