BUG: get_level_values() on int level upcasts to Float64Index if needed #17930

toobaz · 2017-10-20T16:44:26Z

closes MultiIndex.get_leves_values() returns inconsistent Index if there are NaNs #17924
tests added / passed
passes git diff master -u -- "*.py" | flake8 --diff
whatsnew entry

Maybe the check could be more general... but I can't think of any case in which this is needed other than int -> float.

codecov · 2017-10-20T18:26:28Z

Codecov Report

Merging #17930 into master will decrease coverage by 0.01%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master   #17930      +/-   ##
==========================================
- Coverage   91.58%   91.57%   -0.02%     
==========================================
  Files         153      153              
  Lines       51307    51313       +6     
==========================================
- Hits        46991    46988       -3     
- Misses       4316     4325       +9

Flag	Coverage Δ
#multiple	`89.43% <88.88%> (ø)`	⬆️
#single	`40.71% <55.55%> (-0.12%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/numeric.py	`97.33% <88.88%> (+0.07%)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.81% <0%> (-0.1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2db1cc0...a8ad746. Read the comment docs.

jschendel · 2017-10-20T19:29:36Z

doc/source/whatsnew/v0.21.0.txt

@@ -970,6 +970,7 @@ Indexing
 - Fixes ``DataFrame.loc`` for setting with alignment and tz-aware ``DatetimeIndex`` (:issue:`16889`)
 - Avoids ``IndexError`` when passing an Index or Series to ``.iloc`` with older numpy (:issue:`17193`)
 - Allow unicode empty strings as placeholders in multilevel columns in Python 2 (:issue:`17099`)
+- Bug in ``MultiIndex.get_level_values()`` which would return an invalid index on level of ints with missing values (:issue:`17924`)


Could provide a link via :meth:`MultiIndex.get_level_values`

jreback · 2017-10-20T23:05:37Z

pandas/core/indexes/multi.py

-        values = unique._shallow_copy(filled)
+
+        if not unique._can_hold_na and filled.dtype == 'float':
+            values = Index(filled, copy=False, **unique._get_attributes_dict())


no need for all this just use
_shallow_copy_with_infer

jreback · 2017-10-20T23:07:05Z

pandas/tests/indexes/test_multi.py

-        expected = np.array(['a', np.nan, 1], dtype=object)
-        tm.assert_numpy_array_equal(values.values, expected)
+        result = index.get_level_values(0)
+        # workaround for #17929 (should be 'float')


rip out this particular test case and xfail with the correct result
then when fixed it will xpass

jreback · 2017-10-20T23:07:41Z

pandas/tests/indexes/test_multi.py

-        values = index.get_level_values(0)
-        assert values.shape == (0, )
+        result = index.get_level_values(0)
+        assert result.shape == (0, )


u could assert index equal here as well

jreback · 2017-10-21T00:26:45Z

pandas/core/indexes/multi.py

@@ -906,7 +906,12 @@ def _get_level_values(self, level):
        labels = self.labels[level]
        filled = algos.take_1d(unique._values, labels,
                               fill_value=unique._na_value)
-        values = unique._shallow_copy(filled)
+
+        if not unique._can_hold_na and filled.dtype == 'float':


i would always use the infer method no need to have an if

This breaks several tests with dates (e.g. in pandas/tests/test_multilevel.py and pandas/tests/indexes/test_multi.py), in one case because "ValueError: Inferred frequency None from passed dates does not conform to passed frequency D", in other cases because DateTimeIndexes with time zone are _re_added the time zone (if I understand correctly).

you need to see if you can find a general soln. we don't want to have if/then cases all over the place.

The only "general" solution I can think is a property e.g. _na_upcast of Index subclasses (inherited from Index) which is the subclass itself if _can_hold_na = True (that is, all but Int64Index), and is overridden to Float64Index for Int64Index. Then I would just call unique._na_upcast._shallow_copy(filled).

Do you have in mind anything better?

i have no idea ; you need to see why other tests are failing and work around

jreback · 2017-11-23T16:43:55Z

can you rebase and we can revisit

pep8speaks · 2017-12-04T08:41:54Z

Hello @toobaz! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on December 10, 2017 at 18:36 Hours UTC

toobaz · 2017-12-04T09:55:59Z

Sidenote: @pep8speaks highlighted an error ("E129 visually indented line with same indent as next logical line") which neither my local flake8 nor the LINT=true travis run catched. So apparently the two (online) checks run with different configurations (suppressed errors).

jreback · 2017-12-04T11:13:22Z

doc/source/whatsnew/v0.21.0.txt

@@ -1061,6 +1061,7 @@ Indexing
 - Fixes ``DataFrame.loc`` for setting with alignment and tz-aware ``DatetimeIndex`` (:issue:`16889`)
 - Avoids ``IndexError`` when passing an Index or Series to ``.iloc`` with older numpy (:issue:`17193`)
 - Allow unicode empty strings as placeholders in multilevel columns in Python 2 (:issue:`17099`)
+- Bug in :meth:`MultiIndex.get_level_values` which would return an invalid index on level of ints with missing values (:issue:`17924`)


move to 0.22

jreback · 2017-12-04T11:15:52Z

pandas/core/indexes/numeric.py

+                                   not is_integer_dtype(values) and
+                                   isna(values).any()):
+            attributes = self._get_attributes_dict()
+            attributes.update(kwargs)


this is too specific logic, rather call _shallow_copy_with_infer

toobaz · 2017-12-05T11:00:46Z

@jreback ping

jreback

lgtm, do comments. Second would consider (new issue/PR) to split up test_multi.py into a sub-dir as getting pretty long.

jreback · 2017-12-05T11:19:01Z

pandas/core/indexes/numeric.py

@@ -62,6 +62,13 @@ def _maybe_cast_slice_bound(self, label, side, kind):
        # we will try to coerce to integers
        return self._maybe_cast_indexer(label)

+    @Appender(_index_shared_docs['_shallow_copy'])
+    def _shallow_copy(self, values=None, **kwargs):
+        if values is not None and not self._can_hold_na:


can you add a comment here on why we are doing this (yes its obvious to me, but not generally to future readers).

(done)
ping

jreback · 2017-12-05T11:19:32Z

pandas/tests/indexes/test_multi.py

+        tm.assert_index_equal(result, expected)
+
+    def test_get_level_values_all_na(self):
+        arrays = [[np.nan, np.nan, np.nan], ['a', np.nan, 1]]


can you add GH 17924 here as well as a comment

closes pandas-dev#17924

jreback · 2017-12-10T18:36:34Z

resolved conflicts. ping on green.

jreback · 2017-12-10T23:23:16Z

thanks!

jschendel reviewed Oct 20, 2017

View reviewed changes

jreback requested changes Oct 20, 2017

View reviewed changes

toobaz force-pushed the get_level_values_cast branch from 10d7efa to 83a4b0f Compare October 21, 2017 00:07

jreback requested changes Oct 21, 2017

View reviewed changes

toobaz mentioned this pull request Oct 21, 2017

BUG: fix dtype of all-NaN MultiIndex level #17934

Merged

4 tasks

jreback added Bug Dtype Conversions Unexpected or buggy dtype conversions Indexing Related to indexing on series/frames, not to indexes themselves labels Oct 21, 2017

toobaz force-pushed the get_level_values_cast branch from 83a4b0f to 328e175 Compare December 4, 2017 08:41

toobaz force-pushed the get_level_values_cast branch 2 times, most recently from 6fe6775 to 2ffeaa5 Compare December 4, 2017 08:49

jreback requested changes Dec 4, 2017

View reviewed changes

toobaz force-pushed the get_level_values_cast branch from 2ffeaa5 to 7ff2af4 Compare December 4, 2017 16:14

jreback requested changes Dec 5, 2017

View reviewed changes

BUG: get_level_values() on int level upcasts to Float64Index if needed

78d967a

closes pandas-dev#17924

toobaz force-pushed the get_level_values_cast branch from 7ff2af4 to 78d967a Compare December 6, 2017 10:21

jreback added this to the 0.22.0 milestone Dec 10, 2017

jreback added 2 commits December 10, 2017 13:31

Merge branch 'master' into PR_TOOL_MERGE_PR_17930

6779b21

clean

a8ad746

jreback approved these changes Dec 10, 2017

View reviewed changes

jreback merged commit 37086a0 into pandas-dev:master Dec 10, 2017

toobaz deleted the get_level_values_cast branch December 10, 2017 23:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: get_level_values() on int level upcasts to Float64Index if needed #17930

BUG: get_level_values() on int level upcasts to Float64Index if needed #17930

toobaz commented Oct 20, 2017

codecov bot commented Oct 20, 2017 •

edited

Loading

jschendel Oct 20, 2017

jreback Oct 20, 2017

jreback Oct 20, 2017

jreback Oct 20, 2017

jreback Oct 21, 2017

toobaz Oct 21, 2017 •

edited

Loading

jreback Oct 21, 2017

toobaz Oct 21, 2017

jreback Oct 22, 2017

jreback commented Nov 23, 2017

pep8speaks commented Dec 4, 2017 •

edited

Loading

toobaz commented Dec 4, 2017 •

edited

Loading

jreback Dec 4, 2017

jreback Dec 4, 2017

toobaz commented Dec 5, 2017

jreback left a comment

jreback Dec 5, 2017

toobaz Dec 6, 2017

jreback Dec 5, 2017

jreback commented Dec 10, 2017

jreback commented Dec 10, 2017

BUG: get_level_values() on int level upcasts to Float64Index if needed #17930

BUG: get_level_values() on int level upcasts to Float64Index if needed #17930

Conversation

toobaz commented Oct 20, 2017

codecov bot commented Oct 20, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toobaz Oct 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Nov 23, 2017

pep8speaks commented Dec 4, 2017 • edited Loading

Comment last updated on December 10, 2017 at 18:36 Hours UTC

toobaz commented Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toobaz commented Dec 5, 2017

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Dec 10, 2017

jreback commented Dec 10, 2017

codecov bot commented Oct 20, 2017 •

edited

Loading

toobaz Oct 21, 2017 •

edited

Loading

pep8speaks commented Dec 4, 2017 •

edited

Loading

toobaz commented Dec 4, 2017 •

edited

Loading