Repository: pandas-dev/pandas
Pull request: BUG: MultiIndex not dropping nan level and invalid code value
- [x] closes #19387
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

Please take note that another bug is also fixed in this PR:

When the MultiIndex is constructed with code value < -1, a segmentation fault would be resulted in subsequent operations e.g. putting the MultiIndex in a DataFrame
Input:
```python
x = pd.MultiIndex(levels=[['a'], ['x', np.nan]], labels=[[0,-2], [0,1]])
pd.DataFrame(index=x)
```
A segmentation fault can be resulted by these commands


Problem statement:
MultiIndex.dropna() does not always drop NANs
A MultiIndex "label" of -1 means NAN (though I have found no documentation of this).  For example:
```python
>>> pd.MultiIndex.from_arrays([['a', 'a'], ['x', np.nan]])
MultiIndex(levels=[['a'], ['x']],
           labels=[[0, 0], [0, -1]])
```

A MultiIndex can also be constructed with NAN values in levels:
```python
>>> pd.MultiIndex(levels=[['a'], ['x', np.nan]], labels=[[0, 0], [0, 1]])
MultiIndex(levels=[['a'], ['x', nan]],
           labels=[[0, 0], [0, 1]])
```

`MultiIndex.dropna()` works for the first case, but does nothing for the second:
```python
>>> pd.MultiIndex(levels=[['a'], ['x', np.nan]], labels=[[0,0], [0,1]]).dropna()
MultiIndex(levels=[['a'], ['x', nan]],
           labels=[[0, 0], [0, 1]])
```

It appears that `MultiIndex.dropna()` only drops rows whose label is -1, but not rows whose level is actually NAN.  It should drop both types of rows, so the result should be:
```python
MultiIndex(levels=[['a'], ['x']],
           labels=[[0], [0]])
```

I am using Pandas 0.20.3, NumPy 1.13.1,  and Python 3.5.

Hints from issue discussion:
i believe this is fixed in 0.22 or master
pls give a try
@jreback I just checked and it's fixed in neither 0.22 nor master.

Commit message: remove redundant boundary check

Patch to review:
diff --git a/doc/source/whatsnew/v0.25.0.rst b/doc/source/whatsnew/v0.25.0.rst
index 49518c57fc846..b185d9b0e539d 100644
--- a/doc/source/whatsnew/v0.25.0.rst
+++ b/doc/source/whatsnew/v0.25.0.rst
@@ -387,8 +387,8 @@ MultiIndex
 ^^^^^^^^^^
 
 - Bug in which incorrect exception raised by :class:`Timedelta` when testing the membership of :class:`MultiIndex` (:issue:`24570`)
--
--
+- Bug in :class:`Multindex` construction from levels and codes that would incorrectly allows code values < -1
+- Bug in :class:`Multindex` construction from levels and codes that would incorrectly allows NaN levels (:issue:`19387`)
 
 I/O
 ^^^
diff --git a/pandas/core/indexes/multi.py b/pandas/core/indexes/multi.py
index 7a933bdcb0953..390741aaa5800 100644
--- a/pandas/core/indexes/multi.py
+++ b/pandas/core/indexes/multi.py
@@ -243,10 +243,23 @@ def __new__(cls, levels=None, codes=None, sortorder=None, names=None,
 
         if verify_integrity:
             result._verify_integrity()
+
+        codes = [cls._reassign_na_codes(level, code)
+                 for level, code in zip(levels, codes)]
+        result._set_codes(codes, validate=False)
+
         if _set_identity:
             result._reset_identity()
+
         return result
 
+    @classmethod
+    def _reassign_na_codes(cls, level, code):
+        null_mask = isna(level)
+        if np.any(null_mask):
+            code = np.where(null_mask[code], -1, code)
+        return code
+
     def _verify_integrity(self, codes=None, levels=None):
         """
 
@@ -281,6 +294,9 @@ def _verify_integrity(self, codes=None, levels=None):
                                  " level  (%d). NOTE: this index is in an"
                                  " inconsistent state" % (i, level_codes.max(),
                                                           len(level)))
+            if len(level_codes) and level_codes.min() < -1:
+                raise ValueError("On level %d, code value (%d) < -1" %
+                                 (i, level_codes.min()))
             if not level.is_unique:
                 raise ValueError("Level values must be unique: {values} on "
                                  "level {level}".format(
diff --git a/pandas/tests/indexes/multi/test_constructor.py b/pandas/tests/indexes/multi/test_constructor.py
index 639e6aee87c6f..11dd5a7abcde8 100644
--- a/pandas/tests/indexes/multi/test_constructor.py
+++ b/pandas/tests/indexes/multi/test_constructor.py
@@ -67,6 +67,7 @@ def test_constructor_mismatched_codes_levels(idx):
     length_error = (r"On level 0, code max \(3\) >= length of level  \(1\)\."
                     " NOTE: this index is in an inconsistent state")
     label_error = r"Unequal code lengths: \[4, 2\]"
+    code_value_error = (r"On level 0, code value \(-2\) < -1")
 
     # important to check that it's looking at the right thing.
     with pytest.raises(ValueError, match=length_error):
@@ -83,6 +84,23 @@ def test_constructor_mismatched_codes_levels(idx):
     with pytest.raises(ValueError, match=label_error):
         idx.copy().set_codes([[0, 0, 0, 0], [0, 0]])
 
+    # code value smaller than -1
+    with pytest.raises(ValueError, match=code_value_error):
+        MultiIndex(levels=[['a'], ['b']], codes=[[0, -2], [0, 0]])
+
+
+def test_na_levels():
+    tm.assert_index_equal(
+        MultiIndex(levels=[[np.nan, None, pd.NaT, 128, 2]],
+                   codes=[[0, -1, 1, 2, 3, 4]]),
+        MultiIndex(levels=[[np.nan, None, pd.NaT, 128, 2]],
+                   codes=[[-1, -1, -1, -1, 3, 4]]))
+    tm.assert_index_equal(
+        MultiIndex(levels=[[np.nan, 's', pd.NaT, 128, None]],
+                   codes=[[0, -1, 1, 2, 3, 4]]),
+        MultiIndex(levels=[[np.nan, 's', pd.NaT, 128, None]],
+                   codes=[[-1, -1, 1, -1, 3, -1]]))
+
 
 def test_labels_deprecated(idx):
     # GH23752


Code review comments:
diff_hunk: @@ -243,10 +243,23 @@ def __new__(cls, levels=None, codes=None, sortorder=None, names=None,
 
         if verify_integrity:
             result._verify_integrity()
+
+        codes = [cls._reassign_na_codes(level, code)
path: pandas/core/indexes/multi.py
text: @user1:
this should actually be IN _verify_integrity

diff_hunk: @@ -243,10 +243,23 @@ def __new__(cls, levels=None, codes=None, sortorder=None, names=None,
 
         if verify_integrity:
             result._verify_integrity()
+
+        codes = [cls._reassign_na_codes(level, code)
+                 for level, code in zip(levels, codes)]
+        result._set_codes(codes, validate=False)
+
         if _set_identity:
             result._reset_identity()
+
         return result
 
+    @classmethod
+    def _reassign_na_codes(cls, level, code):
path: pandas/core/indexes/multi.py
text: @user1:
call this _validate_codes, pls add a doc-string.

This should be an instance method (and not a classmethod)

@author:
method level changed and moved into validation method

diff_hunk: @@ -387,8 +387,8 @@ MultiIndex
 ^^^^^^^^^^
 
 - Bug in which incorrect exception raised by :class:`Timedelta` when testing the membership of :class:`MultiIndex` (:issue:`24570`)
--
--
+- Bug in :class:`Multindex` construction from levels and codes that would incorrectly allows code values < -1
path: doc/source/whatsnew/v0.25.0.rst
text: @user1:
make this a single note

@author:
notes merged

diff_hunk: @@ -83,6 +84,23 @@ def test_constructor_mismatched_codes_levels(idx):
     with pytest.raises(ValueError, match=label_error):
         idx.copy().set_codes([[0, 0, 0, 0], [0, 0]])
 
+    # code value smaller than -1
+    with pytest.raises(ValueError, match=code_value_error):
+        MultiIndex(levels=[['a'], ['b']], codes=[[0, -2], [0, 0]])
+
+
+def test_na_levels():
+    tm.assert_index_equal(
+        MultiIndex(levels=[[np.nan, None, pd.NaT, 128, 2]],
+                   codes=[[0, -1, 1, 2, 3, 4]]),
+        MultiIndex(levels=[[np.nan, None, pd.NaT, 128, 2]],
+                   codes=[[-1, -1, -1, -1, 3, 4]]))
path: pandas/tests/indexes/multi/test_constructor.py
text: @user1:
use 
result=
expected=
tm.assert_index_equal(result, expected)


@author:
test cases refactored