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

REF: move towards making _apply_blockwise actually block-wise #35730

Merged
merged 11 commits into from
Aug 23, 2020

Conversation

jbrockmendel
Copy link
Member

A step towards #34714, orthogonal to the other outstanding PR in this vein #35696.

@jreback jreback added the Internals Related to non-user accessible pandas implementation label Aug 14, 2020
@jreback jreback added this to the 1.2 milestone Aug 14, 2020
@jbrockmendel
Copy link
Member Author

cc @simonjayhawkins shouldnt mypy recognize Series as satisfying FrameOrSeries?

pandas/core/window/rolling.py:414: error: Incompatible return value type (got "Series", expected "FrameOrSeries")  [return-value]

@simonjayhawkins
Copy link
Member

cc @simonjayhawkins shouldnt mypy recognize Series as satisfying FrameOrSeries?

FrameOrSeries is a typevar, so problems like this involve binding.

so the first error pandas\core\window\rolling.py:414: error: Incompatible return value type (got "Series", expected "FrameOrSeries") [return-value]

FrameOrSeries is the return type of _Window._wrap_results but FrameOrSeries is not bound.

can use FrameOrSeriesUnion as the return type, but if the return type should be the same as passed argument obj then should be bound to obj

i.e.

    def _wrap_results(
        self, results, blocks, obj: FrameOrSeries, exclude=None
    ) -> FrameOrSeries:

but this will still give

pandas\core\window\rolling.py:416: error: Incompatible return value type (got "Series", expected "FrameOrSeries")  [return-value]
pandas\core\window\rolling.py:421: error: Unexpected keyword argument "index" for "NDFrame"  [call-arg]
pandas\core\generic.py:198: note: "NDFrame" defined here
pandas\core\window\rolling.py:421: error: Unexpected keyword argument "columns" for "NDFrame"  [call-arg]
pandas\core\generic.py:198: note: "NDFrame" defined here

since mypy doesn't know that if obj.ndim == 1 refines the type to be Series and other issues need to be addressed since FrameOrSeries is bounded by NDFrame.

for _apply_blockwise, if the return type should be of the same type as self._selected_obj, then should (eventually) create another typevar for this (as FrameOrSeries variable is used for self.obj). However, to bind to a instance variable, need to use Generic, and others won't approve this.

import pandas as pd
from pandas._typing import FrameOrSeries
from typing import Generic


class MyClass(Generic[FrameOrSeries]):
    def __init__(self, obj: FrameOrSeries):
        self._selected_obj = obj

    def foo(self) -> FrameOrSeries:
        return self._selected_obj

reveal_type(MyClass(pd.Series()).foo())
reveal_type(MyClass(pd.DataFrame()).foo())

class MySeries(pd.Series):
    pass

reveal_type(MyClass(MySeries()).foo())
$ mypy ~/mypy-test.py 
C:\Users\simon\mypy-test.py:14: note: Revealed type is 'pandas.core.series.Series*'
C:\Users\simon\mypy-test.py:15: note: Revealed type is 'pandas.core.frame.DataFrame*'
C:\Users\simon\mypy-test.py:20: note: Revealed type is 'mypy-test.MySeries*'

so simplest to get mypy to green is to use FrameOrSeriesUnion for now until we need to refine the types for consistency elsewhere.

@jreback jreback requested a review from mroeschke August 17, 2020 18:30
@jbrockmendel
Copy link
Member Author

Updated+green. Once this and #35740 are in, Ive got a branch ready that goes all the way and makes _apply_blockwise actually blockwise.

@jreback
Copy link
Contributor

jreback commented Aug 21, 2020

if you can rebsae

@jbrockmendel
Copy link
Member Author

rebased+green

@jreback jreback merged commit ee8967f into pandas-dev:master Aug 23, 2020
@jbrockmendel jbrockmendel deleted the reset-dropped-locs-5 branch August 23, 2020 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internals Related to non-user accessible pandas implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants