Include MultiIndex slice in non-reducing slices #19881

fleimgruber · 2018-02-24T15:22:36Z

Changes behaviour of user-passed IndexSlice to return DataFrame instead of
reducing to Series. MultiIndex slices are tuples so this explicitly checks type
and guards with braces.

This seems deceptively simple. WDYT?

closes DataFrame.style.applymap: subset kwarg fails with MultiIndexSlice #19861
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

jreback

can you add the original example (e.g. with the full style usage) as a tests (in the style tests), just as a smoke that it doesn't raise is ok.

pandas/tests/indexing/test_indexing.py

pandas/core/indexing.py

jreback · 2018-02-24T16:36:37Z

also pls add a whatsnew note (bug fix, multiindex is ok)

codecov · 2018-02-24T17:49:48Z

Codecov Report

Merging #19881 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #19881      +/-   ##
==========================================
- Coverage   92.32%   92.32%   -0.01%     
==========================================
  Files         166      166              
  Lines       52440    52429      -11     
==========================================
- Hits        48417    48404      -13     
- Misses       4023     4025       +2

Flag	Coverage Δ
#multiple	`90.74% <100%> (-0.01%)`	⬇️
#single	`43.01% <0%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/indexing.py	`93.87% <100%> (ø)`	⬆️
pandas/core/missing.py	`91.7% <0%> (-0.6%)`	⬇️
pandas/core/arrays/datetimelike.py	`96.44% <0%> (-0.17%)`	⬇️
pandas/core/arrays/period.py	`98.59% <0%> (+0.06%)`	⬆️

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 cf92230...a00dd9e. Read the comment docs.

fleimgruber · 2018-02-24T18:57:34Z

Thanks for review, changed as requested, but still two minor questions for clarification (comment)

jreback · 2018-11-01T01:39:34Z

this got a bit lost. can you rebase and move the note to 0.24.0

pep8speaks · 2018-11-01T08:47:16Z

Hello @fleimgruber! Thanks for updating the PR.

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

Comment last updated on January 02, 2019 at 00:28 Hours UTC

fleimgruber · 2018-11-01T10:22:28Z

Rebased and whatsnew 0.24.0.

alimcmaster1 · 2018-11-15T22:18:54Z

@fleimgruber mind rebasing and resolving the conflicts above. ( The build issues on azure you are seeing is resolved on master )

fleimgruber · 2018-11-21T20:27:35Z

@alimcmaster1 rebased. Which conflicts were you referring to? Still failing on Azure - a new bug?

alimcmaster1 · 2018-11-21T23:16:41Z

@fleimgruber thanks ( you have resolved the conflicts now ). It appears to be a flakey test, i've raised a PR here #23849 to hopefully fix the problem

alimcmaster1 · 2018-11-22T19:00:23Z

@jreback @gfyoung @mroeschke does someone mind restarting the azure 27 build on this PR - seems to be a flakey test. Im look in #23849

jreback · 2018-12-23T23:15:47Z

pls merge master

fleimgruber · 2018-12-27T12:27:04Z

Rebased on master.

mroeschke

One small note otherwise LGTM.

mroeschke · 2018-12-27T18:18:05Z

pandas/core/indexing.py

-        return isinstance(part, slice) or is_list_like(part)
+        # true when slice does *not* reduce, False when part is a tuple,
+        # i.e. MultiIndex slice
+        return (isinstance(part, slice) or is_list_like(part))\


We are moving away from using \ line breaks in the codebase. Could you wrap this condition in parenthesis instead?

mroeschke · 2018-12-27T18:18:39Z

pandas/tests/io/formats/test_style.py

@@ -274,6 +274,31 @@ def f(x):
                        col in self.df.loc[slice_].columns}
            assert result == expected

+    def test_applymap_subset_multiindex(self):
+        # https://github.com/pandas-dev/pandas/issues/19861


Can you leave a comment that this is just a smoke test.

pandas/tests/indexing/test_indexing.py

jreback · 2019-01-01T01:13:04Z

@fleimgruber can you update for comments

Changes behaviour of user-passed IndexSlice to return DataFrame instead of reducing to Series. MultiIndex slices are tuples so this explicitly checks type and guards with parentheses. Fixes pandas-dev#19861

jreback · 2019-01-02T01:16:02Z

thanks @fleimgruber

* upstream/master: Include MultiIndex slice in non-reducing slices (pandas-dev#19881)

jreback added Indexing Related to indexing on series/frames, not to indexes themselves Output-Formatting __repr__ of pandas objects, to_string IO HTML read_html, to_html, Styler.apply, Styler.applymap MultiIndex labels Feb 24, 2018

jreback requested changes Feb 24, 2018

View reviewed changes

pandas/tests/indexing/test_indexing.py Outdated Show resolved Hide resolved

jreback requested changes Feb 24, 2018

View reviewed changes

pandas/tests/indexing/test_indexing.py Outdated Show resolved Hide resolved

jreback reviewed Feb 24, 2018

View reviewed changes

pandas/core/indexing.py Outdated Show resolved Hide resolved

fleimgruber force-pushed the style_applymap_multiindex branch from 2370e0b to eaa9dba Compare November 1, 2018 08:47

fleimgruber force-pushed the style_applymap_multiindex branch from eaa9dba to 0d7abf3 Compare November 1, 2018 08:53

fleimgruber force-pushed the style_applymap_multiindex branch 2 times, most recently from da6c250 to 3d3b80f Compare November 21, 2018 18:48

fleimgruber force-pushed the style_applymap_multiindex branch from 3d3b80f to 10976fd Compare December 27, 2018 10:58

mroeschke approved these changes Dec 27, 2018

View reviewed changes

mroeschke reviewed Dec 27, 2018

View reviewed changes

jreback requested changes Dec 27, 2018

View reviewed changes

pandas/tests/indexing/test_indexing.py Show resolved Hide resolved

jreback added this to the 0.24.0 milestone Dec 27, 2018

fleimgruber added 4 commits January 2, 2019 01:27

Include MultiIndex slice in non-reducing slices

540471b

Changes behaviour of user-passed IndexSlice to return DataFrame instead of reducing to Series. MultiIndex slices are tuples so this explicitly checks type and guards with parentheses. Fixes pandas-dev#19861

Use tm.assert_frame_equal and smoke test from GH issue

f00fa73

whatsnew and changes requested

0885c79

Update comments and parenthesis for if condition

a00dd9e

fleimgruber force-pushed the style_applymap_multiindex branch from 5b5cdd8 to a00dd9e Compare January 2, 2019 00:28

jreback approved these changes Jan 2, 2019

View reviewed changes

jreback merged commit 71efe61 into pandas-dev:master Jan 2, 2019

thoo added a commit to thoo/pandas that referenced this pull request Jan 2, 2019

Merge remote-tracking branch 'upstream/master' into isort-io

663cb64

* upstream/master: Include MultiIndex slice in non-reducing slices (pandas-dev#19881)

simonjayhawkins mentioned this pull request Jan 2, 2019

TST: move test_non_reducing_slice_on_multiindex #24545

Merged

fleimgruber deleted the style_applymap_multiindex branch January 2, 2019 09:40

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

Include MultiIndex slice in non-reducing slices (pandas-dev#19881)

df72f31

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

Include MultiIndex slice in non-reducing slices (pandas-dev#19881)

237e674

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include MultiIndex slice in non-reducing slices #19881

Include MultiIndex slice in non-reducing slices #19881

fleimgruber commented Feb 24, 2018 •

edited

Loading

jreback left a comment

jreback commented Feb 24, 2018

codecov bot commented Feb 24, 2018 •

edited

Loading

fleimgruber commented Feb 24, 2018

jreback commented Nov 1, 2018

pep8speaks commented Nov 1, 2018 •

edited

Loading

fleimgruber commented Nov 1, 2018

alimcmaster1 commented Nov 15, 2018

fleimgruber commented Nov 21, 2018

alimcmaster1 commented Nov 21, 2018

alimcmaster1 commented Nov 22, 2018

jreback commented Dec 23, 2018

fleimgruber commented Dec 27, 2018 •

edited

Loading

mroeschke left a comment

mroeschke Dec 27, 2018

mroeschke Dec 27, 2018

jreback commented Jan 1, 2019

jreback commented Jan 2, 2019

Include MultiIndex slice in non-reducing slices #19881

Include MultiIndex slice in non-reducing slices #19881

Conversation

fleimgruber commented Feb 24, 2018 • edited Loading

jreback left a comment

Choose a reason for hiding this comment

jreback commented Feb 24, 2018

codecov bot commented Feb 24, 2018 • edited Loading

Codecov Report

fleimgruber commented Feb 24, 2018

jreback commented Nov 1, 2018

pep8speaks commented Nov 1, 2018 • edited Loading

Comment last updated on January 02, 2019 at 00:28 Hours UTC

fleimgruber commented Nov 1, 2018

alimcmaster1 commented Nov 15, 2018

fleimgruber commented Nov 21, 2018

alimcmaster1 commented Nov 21, 2018

alimcmaster1 commented Nov 22, 2018

jreback commented Dec 23, 2018

fleimgruber commented Dec 27, 2018 • edited Loading

mroeschke left a comment

Choose a reason for hiding this comment

mroeschke Dec 27, 2018

Choose a reason for hiding this comment

mroeschke Dec 27, 2018

Choose a reason for hiding this comment

jreback commented Jan 1, 2019

jreback commented Jan 2, 2019

fleimgruber commented Feb 24, 2018 •

edited

Loading

codecov bot commented Feb 24, 2018 •

edited

Loading

pep8speaks commented Nov 1, 2018 •

edited

Loading

fleimgruber commented Dec 27, 2018 •

edited

Loading