-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: df.iloc[:, :1] with EA column #32959
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
im now thinking the test doesnt belong in extension tests; if it were to fail (e.g. in the status quo), that wouldnt indicate that the EA author needed to fix somthing |
d0a68fb
to
628e242
Compare
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. @jorisvandenbossche
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.
see earlier comments
i think comments have been addressed @jorisvandenbossche, can you confirm? |
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 add some more test cases (the other possibilities I mentioned, with xfails for now if need be)
pandas/core/internals/blocks.py
Outdated
raise AssertionError( | ||
"invalid slicing for a 1-ndim ExtensionArray", first | ||
) | ||
elif not (first == slice(None) or first == slice(1)): |
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 think the check for slice(1)
is too strict. Eg, you can slice with out of bounds ends, like slice(2)
, and that should work as well
# return same dims as we currently have | ||
if not isinstance(slicer, tuple) and self.ndim == 2: | ||
# reached via getitem_block via _slice_take_blocks_ax0 |
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.
Could also put this check in getitem_block
or _slice_take_blocks_ax0
to ensure that _slice
always gets the correct tuple of slices?
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.
Or, directly check the value? As here you convert it to a len-2 tuple, and then in a next step, you convert the tuple back to a slice?
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.
Hmm one way to avoid this PITA would be to have 2D EAs...
@jorisvandenbossche have your concerns/comments been addressed here? |
gentle ping @jorisvandenbossche |
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.
minor test comment
result = df.iloc[:, :1] | ||
self.assert_frame_equal(result, df) | ||
|
||
result = df.iloc[:, :2] |
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 add some df.iloc[:, -1:]
as well
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.
updated+green
needs a rebase as well |
thanks @jbrockmendel |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff