Skip to content

BUG: Fixed PandasArray.__setitem__ with str #28119

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Sep 17, 2019
Merged

BUG: Fixed PandasArray.__setitem__ with str #28119

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f0718fa
BUG: Fixed PandasArray.__setitem__ with str
TomAugspurger Aug 23, 2019
75c2c58
no casting
TomAugspurger Sep 9, 2019
f482d99
Merge remote-tracking branch 'upstream/master' into PandasArray-setit…
TomAugspurger Sep 9, 2019
6b8dfe2
Merge remote-tracking branch 'upstream/master' into PandasArray-setit…
TomAugspurger Sep 10, 2019
1828890
whatsnews
TomAugspurger Sep 10, 2019
9d77af5
Merge remote-tracking branch 'upstream/master' into PandasArray-setit…
TomAugspurger Sep 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion doc/source/whatsnew/v1.0.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ Build Changes
ExtensionArray
^^^^^^^^^^^^^^

-
- Bug in :class:`arrays.PandasArray` when setting a scalar string (:issue:`28118`)
-


Expand Down
9 changes: 8 additions & 1 deletion pandas/core/arrays/numpy_.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from pandas.util._decorators import Appender
from pandas.util._validators import validate_fillna_kwargs

from pandas.core.dtypes.common import is_object_dtype
from pandas.core.dtypes.dtypes import ExtensionDtype
from pandas.core.dtypes.generic import ABCIndexClass, ABCSeries
from pandas.core.dtypes.inference import is_array_like, is_list_like
Expand Down Expand Up @@ -236,7 +237,13 @@ def __setitem__(self, key, value):
value = np.asarray(value)

values = self._ndarray
t = np.result_type(value, values)
if isinstance(value, str):
if is_object_dtype(self.dtype._dtype):
t = np.dtype(object)
else:
t = self.dtype._dtype
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a test that hits this branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test_setitem_object_typecode[None] hits it (setting a string into an integer array).

Copy link
Contributor

Choose a reason for hiding this comment

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

simpler to leave the original code then just convert a np.str to no.object (which is what we do inside blocks manager and other places); maybe have a function to do this rather than rewriting logic all over the place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that's appropriate for PandasArray. The idea is to take an arbitrary numpy array and box it in an extension array.

Copy link
Contributor

Choose a reason for hiding this comment

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

and that’s exactly what is done in ObjectBlock now

pls refactor rather than adding logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I doubt it. I think I was mimicking the behavior of Series.__setitem__

In [4]: x = np.array([1, 2, 3])

In [5]: s = pd.Series(x)

In [6]: s.values is x
Out[6]: True

In [7]: s[0] = 'a'

In [8]: s.values is x
Out[8]: False

But I'm happy to be stricter here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That said, we'll also inherit things like

In [11]: x = np.array([1, 2, 3])

In [12]: x[0] = 5.5

In [13]: x
Out[13]: array([5, 2, 3])

But maybe that's OK, if the intent is to be close to NumPy here.

Copy link
Member

Choose a reason for hiding this comment

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

To what extent can we punt on the float treatment for now? I think there's a case to be made that we should raise instead of casting there, but don't want to bog this down any more.

Copy link
Contributor Author

@TomAugspurger TomAugspurger Sep 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm I think our options are to always raise when the dtypes don't match, or adopt NumPy's behavior. I don't think I have a preference.

Copy link
Member

Choose a reason for hiding this comment

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

The thought that pushes me towards raising is that if/when this is backing a Block, we want Block.setitem to try to set it on block.values and then fall back to casting.

else:
t = np.result_type(value, values)
if t != self._ndarray.dtype:
values = values.astype(t, casting="safe")
values[key] = value
Expand Down
8 changes: 8 additions & 0 deletions pandas/tests/arrays/test_numpy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,3 +211,11 @@ def test_basic_binop():
result = x + x
expected = PandasArray(np.array([2, 4, 6]))
tm.assert_extension_array_equal(result, expected)


@pytest.mark.parametrize("dtype", [None, object])
def test_setitem_object_typecode(dtype):
arr = PandasArray(np.array(["a", "b", "c"], dtype=dtype))
arr[0] = "t"
expected = PandasArray(np.array(["t", "b", "c"], dtype=dtype))
tm.assert_extension_array_equal(arr, expected)