Repository: pandas-dev/pandas
Pull request: TST: Add test for setting item using python list (#19406)
- [X] closes #19406 
- [X] tests added / passed
- [X] passes `black pandas`
- [X] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [NA] whatsnew entry


Problem statement:
Boolean indexing and assignment from python list broken
There is a problem with assigning lists to indexed Series
```python
from math import nan
import pandas as pd
s1 = pd.Series([nan, nan, "c"])
s1[s1.isnull()] = ["a","b"]
s2 = pd.Series([nan, "b", nan])
s2[s2.isnull()] = ["a","c"]
#s1: "a", "b", "c"
#s2: "a", "b", "a"
```

`s1` shows the correct result, whereas `s2` is obviously wrong. In pandas `0.19.2` the result is correct, but in `0.21.0` and `0.23.0.dev0+97.g24c07b075` I get the above behavior. It can be fixed if a numpy array instead of a python list is assigned.

```python
from math import nan
import pandas as pd
import numpy as np
s1 = pd.Series([nan, nan, "c"])
s1[s1.isnull()] = np.array(["a","b"])
s2 = pd.Series([nan, "b", nan])
s2[s2.isnull()] = np.array(["a","c"])
#s1: "a", "b", "c"
#s2: "a", "b", "c"
```

There probably should be a test case to catch that...

#### Output of ``pd.show_versions()``
<details>

INSTALLED VERSIONS
------------------
commit: None
python: 3.5.4.final.0
python-bits: 64
OS: Windows
OS-release: 10
machine: AMD64
processor: Intel64 Family 6 Model 78 Stepping 3, GenuineIntel
byteorder: little
LC_ALL: None
LANG: en_US
LOCALE: None.None

pandas: 0.23.0.dev0+97.g24c07b075
pytest: 3.2.3
pip: 9.0.1
setuptools: 38.2.4
Cython: 0.27.3
numpy: 1.14.0
scipy: 1.0.0
pyarrow: None
xarray: None
IPython: 6.2.1
sphinx: None
patsy: None
dateutil: 2.6.1
pytz: 2017.3
blosc: None
bottleneck: None
tables: None
numexpr: None
feather: None
matplotlib: 2.0.0
openpyxl: None
xlrd: None
xlwt: None
xlsxwriter: None
lxml: None
bs4: 4.6.0
html5lib: 0.9999999
sqlalchemy: 1.1.13
pymysql: 0.7.11.None
psycopg2: None
jinja2: 2.10
s3fs: None
fastparquet: None
pandas_gbq: None
pandas_datareader: None
</details>

Hints from issue discussion:
This works now as intended. May need some tests
Can confirm it's working with pandas 1.1.3
take
I'm trying to decide where to put this test. I'm looking at `pandas/tests/indexing` folder and thinking of putting it `pandas/tests/indexing/test_na_indexing.py`...?
That would be better suited in the boolean group. Isnull is here just a tool to create a boolean mask. Could you maybe search for related tests? Maybe in series/indexing/test_setitem?
> Isnull is here just a tool to create a boolean mask.

I just realized this while I was typing out the test. 😅 

Will do and thanks for the guidance!

Commit message: TST: Add test for setting item using python list (#19406)

Patch to review:
diff --git a/pandas/tests/series/indexing/test_setitem.py b/pandas/tests/series/indexing/test_setitem.py
index 4f00f9c931d6a..a412240e7ee45 100644
--- a/pandas/tests/series/indexing/test_setitem.py
+++ b/pandas/tests/series/indexing/test_setitem.py
@@ -126,6 +126,14 @@ def test_setitem_boolean_different_order(self, string_series):
 
         tm.assert_series_equal(copy, expected)
 
+    def test_setitem_boolean_using_python_list(self):
+        # GH#19406
+        series = Series([None, "b", None])
+        mask = series.isnull()
+        series[mask] = ["a", "c"]
+        expected = Series(["a", "b", "c"])
+        tm.assert_series_equal(series, expected)
+
     @pytest.mark.parametrize("value", [None, NaT, np.nan])
     def test_setitem_boolean_td64_values_cast_na(self, value):
         # GH#18586


Code review comments:
diff_hunk: @@ -126,6 +126,14 @@ def test_setitem_boolean_different_order(self, string_series):
 
         tm.assert_series_equal(copy, expected)
 
+    def test_setitem_boolean_using_python_list(self):
+        # GH#19406
+        series = Series([None, "b", None])
path: pandas/tests/series/indexing/test_setitem.py
text: @user1:
call it `s`

@user2:
Please something like ser, no Single letter names

diff_hunk: @@ -126,6 +126,14 @@ def test_setitem_boolean_different_order(self, string_series):
 
         tm.assert_series_equal(copy, expected)
 
+    def test_setitem_boolean_using_python_list(self):
+        # GH#19406
+        series = Series([None, "b", None])
path: pandas/tests/series/indexing/test_setitem.py
text: @user1:
Here in the test you have only ``None``, but there are also ``np.nan``, ``math.nan``, ``pd.NA``, ``pd.NaN``, ``pd.NaT`` (probably something else?).
I guess it should work with them too, shouldn't it?

@author:
I was wondering that. I wonder though if that's unnecessary since the test is mainly about using the python list. The null values were only for masking...?

@author:
Maybe once I resolve @user2 comment about hardcoding the mask instead of calling isnull() that would be clearer?

@user1:
Sounds good.

diff_hunk: @@ -126,6 +126,14 @@ def test_setitem_boolean_different_order(self, string_series):
 
         tm.assert_series_equal(copy, expected)
 
+    def test_setitem_boolean_using_python_list(self):
+        # GH#19406
+        series = Series([None, "b", None])
+        mask = series.isnull()
path: pandas/tests/series/indexing/test_setitem.py
text: @user1:
just hardcode the mask
```
mask = [0, 2]
```
you can include `assert mask == series.isnull()` if you like