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

Fix failing tests with Pandas 1.3.0 (was: Add logic to deal with ABCIndexClass being renamed to ABCIndex) #218

Merged
merged 8 commits into from Jul 7, 2021

Conversation

frreiss
Copy link
Member

@frreiss frreiss commented Jul 3, 2021

Pandas 1.3.x renamed its abstract base class for indexes from ABCIndexClass to ABCIndex, which is messing up some of our type checks. This PR adds some logic to work around that renaming. I'm using exceptions as control flow, which is a bit ugly, but the alternatives are also ugly.

@frreiss
Copy link
Member Author

frreiss commented Jul 3, 2021

Okay, that didn't work. Will try something else.

@frreiss
Copy link
Member Author

frreiss commented Jul 6, 2021

Fixed some bugs in SpanArray and TokenSpanArray that new regression tests in Pandas 1.3.0 brought to light. Now we're down to 62 failing tests.

@frreiss
Copy link
Member Author

frreiss commented Jul 7, 2021

Update: Most of the test failures seem to be due to a bug in Pandas 1.3.0. Due to some performance optimizations introduced in pandas-dev/pandas#40353, Pandas turns DataFrame.iloc[slice(x, y, z)] into __getitem__((..., slice(x, y, z)) on the ExtensionArray that backs any column defined with an extension type.

Code to reproduce:

from pandas.api.extensions import ExtensionArray,ExtensionDtype

class MyExtensionDtype(ExtensionDtype):
    """Minimal extension dtype"""
    def __init__(self):
        pass
    
    @property
    def type(self):
        return int
    
    @property
    def name(self) -> str:
        return "MyExtensionDtype"
        
    @classmethod
    def construct_array_type(cls):
        return MyExtensionArray()

class MyExtensionArray(ExtensionArray, ExtensionScalarOpsMixin):
    """Minimal extension array that logs calls to __getitem__()"""
    @property
    def dtype(self):
        return MyExtensionDtype()
    
    def copy(self):
        return MyExtensionArray()
    
    def __len__(self):
        return 5
    
    def __getitem__(self, key):
        print(f"__getitem__ called with key '{key}'")
        return 42

arr = MyExtensionArray()
df = pd.DataFrame({"a": arr})
_ = df.iloc[:3]

which prints out:

__getitem__ called with key '(Ellipsis, slice(None, 3, None))'

It should print the following instead:

__getitem__ called with key 'slice(None, 3, None)'

I'll put in a workaround tomorrow and file a bug with Pandas.

FYI @BryanCutler @ZachEichen @PokkeFe @Crushellini

@frreiss
Copy link
Member Author

frreiss commented Jul 7, 2021

Update:

@frreiss
Copy link
Member Author

frreiss commented Jul 7, 2021

Update: Fixed another minor bug. Now we are down to 49 failing tests.

@frreiss
Copy link
Member Author

frreiss commented Jul 7, 2021

Update:

Now we're down to 2 failing tests.

@frreiss frreiss changed the title Add logic to deal with ABCIndexClass being renamed to ABCIndex Fix failing tests with Pandas 1.3.0 (was: Add logic to deal with ABCIndexClass being renamed to ABCIndex) Jul 7, 2021
@frreiss
Copy link
Member Author

frreiss commented Jul 7, 2021

All tests passing against Pandas 1.3.0 now. Merging this PR to unblock other PRs.

@frreiss frreiss merged commit 1a002c0 into CODAIT:master Jul 7, 2021
Copy link
Member

@BryanCutler BryanCutler left a comment

Choose a reason for hiding this comment

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

LGTM, just a minor suggestion

# ABCIndexClass changed to ABCIndex in Pandas 1.3
# noinspection PyUnresolvedReferences
from pandas.core.dtypes.generic import ABCIndex
ABCIndexClass = ABCIndex
Copy link
Member

Choose a reason for hiding this comment

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

You could do from pandas.core.dtypes.generic import ABCIndex as ABCIndexClass

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I should have thought of that. Will put in a follow-on PR with that edit.

frreiss added a commit to frreiss/tep-fred that referenced this pull request Jul 8, 2021
frreiss added a commit that referenced this pull request Jul 8, 2021
Minor: Address review comments from #218
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants