-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
BUG: Array.__setitem__ failing with nullable boolean mask #31484
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 generally good
doc/source/whatsnew/v1.0.1.rst
Outdated
@@ -70,6 +70,7 @@ Indexing | |||
|
|||
- | |||
- | |||
- Bug in ``__setitem__`` in ``Array`` which fails with nullable boolean mask (:issue:`31446`) |
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.
- Bug in ``__setitem__`` in ``Array`` which fails with nullable boolean mask (:issue:`31446`) | |
- Bug where assigning to a Series using a IntegerArray / BooleanArray as a mask would raise ``TypeError`` (:issue:`31446`) |
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 some comments.
Those are not strictly needed to fix the regression (only nullable integer already existed before), but I think it could be good to fix this more generally
@charlesdong1991 I haven't looked closely, but you may need to skip that setitem test for PandasArray. There are some issues with setting sized values in an ndarray. |
ahh, thanks a lot for letting me know!! @TomAugspurger i am indeed struggling with one failed test which is caused by I will skip this for now, and pls let me know if you think a new issue needs to be created or not. just FYI, the only failed test
|
doc/source/whatsnew/v1.0.1.rst
Outdated
@@ -70,6 +70,7 @@ Indexing | |||
|
|||
- | |||
- | |||
- Bug where assigning to a ``Series`` using a IntegerArray / BooleanArray as a mask would raise ``TypeError`` (:issue:`31446`) |
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.
:class:`Series`
instead of
``Series``
so that there's a link in the docs?
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.
yeah, tbh i wasn't quite sure about the rule here, since i see three different representations used in whatsnew
:class:`Series`, ``Series``, Series
but seems having :class:Series
is more consistent, will change
|
||
def test_setitem_nullable_mask(self, data): | ||
# GH 31446 | ||
# TODO: there is some issue with PandasArray, therefore, |
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 create an issue for this? (e.g. PandasArray support for setitem)
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.
will do
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.
@charlesdong1991 is there an opened issue for this yet? I haven't found it and it causes CI of geopandas to fail as contrary to the comment this test is not skipped.
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.
@martinfleis The issue for geopandas is not due to PandasArray not working, but rather because we need to update our __setitem__
implementation (similar to how I updated the __getitem__
), will do a PR for that.
thanks @charlesdong1991 |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff