CLN: Remove Legacy MultiIndex Index Compatibility #21740

elmq0022 · 2018-07-05T01:06:55Z

closes TST/CLN: remove MultiIndex compat #21654
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
what's new - removing legacy MultiIndex internal formats

jreback

pls add a whatsnew note - removing legacy MuktiIndex internal formats

jreback · 2018-07-05T01:12:29Z

pandas/tests/indexes/multi/test_conversion.py

-def test_legacy_pickle(datapath):
-
-    path = datapath('indexes', 'multi', 'data', 'multiindex_v1.pickle')
-    obj = pd.read_pickle(path)


can u also remove the legacy code itself from MultiIndex

So, I came to the conclusion that I can just delete the __setstate__ method as there is no associated __getstate__ method. I hope this approach is correct.

codecov · 2018-07-05T03:33:56Z

Codecov Report

Merging #21740 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #21740      +/-   ##
==========================================
+ Coverage   92.05%   92.05%   +<.01%     
==========================================
  Files         170      170              
  Lines       50708    50697      -11     
==========================================
- Hits        46677    46668       -9     
+ Misses       4031     4029       -2

Flag	Coverage Δ
#multiple	`90.46% <ø> (ø)`	⬆️
#single	`42.36% <ø> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/multi.py	`95.25% <ø> (+0.12%)`	⬆️

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 7a2fbce...ae446bc. Read the comment docs.

elmq0022 · 2018-07-05T15:26:43Z

I read the pickle docs and I believe I have it figured out. Will play with it a little tonight and see if I have it right. Thanks!

…

On Wed, Jul 4, 2018, 8:13 PM Jeff Reback ***@***.***> wrote: ***@***.**** requested changes on this pull request. pls add a whatsnew note - removing legacy MuktiIndex internal formats ------------------------------ In pandas/tests/indexes/multi/test_conversion.py <#21740 (comment)>: > @@ -87,46 +85,6 @@ def test_to_hierarchical(): assert result.names == index.names ***@***.***(PY3, reason="testing legacy pickles not support on py3") -def test_legacy_pickle(datapath): - - path = datapath('indexes', 'multi', 'data', 'multiindex_v1.pickle') - obj = pd.read_pickle(path) can u also remove the legacy code itself from MultiIndex — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#21740 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADSn4HQ-X3K-J6n2M_P0bph0ug9GKnNLks5uDWgwgaJpZM4VDHzu> .

jreback · 2018-07-06T03:18:23Z

pandas/core/indexes/multi.py

-            levels = state.get('levels')
-            labels = state.get('labels')
-            sortorder = state.get('sortorder')
-            names = state.get('names')


hmm i don't think you can remove this.

jreback · 2018-07-06T03:18:37Z

pandas/core/indexes/multi.py

-
-        elif isinstance(state, tuple):
-
-            nd_state, own_state = state


there are the _v1 and _v2 properties you can remove though

also the _bounds, & get_major_bounds are not used either (I may have a separate issue number for that)

elmq0022 · 2018-07-14T21:15:18Z

@jreback I am not sure what happened with the circleci build. I could use some help with the postmortem as I haven't dealt with circleci directly before.

jbrockmendel · 2018-07-19T03:28:00Z

pandas/tests/indexes/multi/test_operations.py

-def test_bounds(idx):
-    idx._bounds
+# def test_bounds(idx):
+#     idx._bounds


If this is no longer needed, just delete.

jbrockmendel · 2018-07-19T03:29:57Z

Looks like circleCI fail may be unrelated. Pushing a new commit will have it try again.

elmq0022 · 2018-07-19T03:40:23Z

Thanks. The queue seemed really full that day. Will give it another go.

…

On Wed, Jul 18, 2018, 10:30 PM jbrockmendel ***@***.***> wrote: Looks like circleCI fail may be unrelated. Pushing a new commit will have it try again. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#21740 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADSn4NcbynbIJtxzI9UiY9EefKrDYDtZks5uH_1JgaJpZM4VDHzu> .

jreback · 2018-07-20T13:24:37Z

doc/source/whatsnew/v0.24.0.txt

@@ -444,7 +444,7 @@ Missing
 MultiIndex
 ^^^^^^^^^^

-
+- Removing legacy MultiIndex internal formats


add the issue number

Okay, will add the issue number tonight and resubmit. Looks like the first failure was something on AppVeyor's side.

Also, the above message is unclear to me what it is about (as an external reader, didn't look at the rest of the PR)

elmq0022 · 2018-07-24T15:25:30Z

@jreback I think this is good to go.

elmq0022 · 2018-07-25T11:31:46Z

@jreback I rebased after the latest merge should be good to go. Thanks!

jreback

minor comment. ping on green.

jreback · 2018-07-25T12:02:28Z

pandas/tests/indexes/multi/test_analytics.py

@@ -15,8 +15,8 @@ def test_shift(idx):
    pytest.raises(NotImplementedError, idx.shift, 1, 2)


-def test_bounds(idx):
-    idx._bounds
+# def test_bounds(idx):


remote this entirely

elmq0022 · 2018-07-25T12:08:13Z

Nice catch. Thanks! Will get to this tonight.

…

On Wed, Jul 25, 2018, 7:04 AM Jeff Reback ***@***.***> wrote: ***@***.**** requested changes on this pull request. minor comment. ping on green. ------------------------------ In pandas/tests/indexes/multi/test_analytics.py <#21740 (comment)>: > @@ -15,8 +15,8 @@ def test_shift(idx): pytest.raises(NotImplementedError, idx.shift, 1, 2) -def test_bounds(idx): - idx._bounds +# def test_bounds(idx): remote this entirely — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#21740 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADSn4NbKh6ER0ZklowaIbunz2VBbRoPbks5uKF67gaJpZM4VDHzu> .

jorisvandenbossche · 2018-07-25T12:20:40Z

doc/source/whatsnew/v0.24.0.txt

@@ -513,7 +513,7 @@ Missing
 MultiIndex
 ^^^^^^^^^^

-
+- Removed legacy internal formats (:issue:`21654`)


Putting it here again: the above message is unclear to me what it is about.

Either this change is not relevant for the user (eg only internal clean-up), and not whatsnew notice is needed. Or it is, but then needs to be clarified (eg do we drop support for very old pickle files?)

I don't think we have tests for really old pickle files You could add a note that this is circa 0.8.0, where we only maintain back compat for pickle to 0.13 (IIRC)

if that is the case, I would maybe just remove the what's new notice? (since the 0.13 threshold is already documented I assume?)

yeah I don't think this has any user visible effects, but still having a whatsnew note just documents what we did. I would expand the note slightly but leave it.

We don't always add a whatsnew note for internal code clean-up (if it has no user visible change), so I still don't think it is needed here. Anyhow, if we keep one, it should be comprehensible for users (the current one is not even comprehensible for me without looking at the issue)

elmq0022 · 2018-07-26T13:37:07Z

Okay thanks. It's a fair point. I will try to summarize the issue better in the what's new text and resubmit.

…

On Thu, Jul 26, 2018, 8:17 AM Joris Van den Bossche < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In doc/source/whatsnew/v0.24.0.txt <#21740 (comment)>: > @@ -513,7 +513,7 @@ Missing MultiIndex ^^^^^^^^^^ -- +- Removed legacy internal formats (:issue:`21654`) We don't always add a whatsnew note for internal code clean-up (if it has no user visible change), so I still don't think it is needed here. Anyhow, if we keep one, it should be comprehensible for users (the current one is not even comprehensible for me without looking at the issue) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#21740 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADSn4F1x-6KqhBcrsUpZn60_yUVYDcobks5uKcFagaJpZM4VDHzu> .

elmq0022 · 2018-07-27T11:30:14Z

@jreback everything is green. Thank you for all your help on refactoring this MultiIndex text suite.

jorisvandenbossche

Thanks for the update, that's clear now!

elmq0022 · 2018-07-27T12:18:00Z

No problem. I'm glad you brought up the issue. Thanks!

…

On Fri, Jul 27, 2018, 6:33 AM Joris Van den Bossche < ***@***.***> wrote: ***@***.**** approved this pull request. Thanks for the update, that's clear now! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#21740 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADSn4DDTJd60km7OCfxlcxkDJhQOdO8Aks5uKvp6gaJpZM4VDHzu> .

* master: BENCH: asv csv reading benchmarks no longer read StringIO objects off the end (pandas-dev#21807) BUG: df.agg, df.transform and df.apply use different methods when axis=1 than when axis=0 (pandas-dev#21224) BUG: bug in GroupBy.count where arg minlength passed to np.bincount must be None for np<1.13 (pandas-dev#21957) CLN: Vbench to asv conversion script (pandas-dev#22089) consistent docstring (pandas-dev#22066) TST: skip pytables test with not-updated pytables conda package (pandas-dev#22099) CLN: Remove Legacy MultiIndex Index Compatibility (pandas-dev#21740) DOC: Reword doc for filepath_or_buffer in read_csv (pandas-dev#22058) BUG: rolling with MSVC 2017 build (pandas-dev#21813)

jreback requested changes Jul 5, 2018

View reviewed changes

gfyoung added Testing pandas testing functions or related to the test suite MultiIndex Clean Compat pandas objects compatability with Numpy or Python functions labels Jul 5, 2018

jreback changed the title ~~fix issue #21654~~ CLN: remove legacy MultiIndex #21654 Jul 5, 2018

elmq0022 force-pushed the remove_multiindex_pickles branch from 10c8a23 to 12a612e Compare July 6, 2018 02:41

jreback requested changes Jul 6, 2018

View reviewed changes

elmq0022 force-pushed the remove_multiindex_pickles branch from 12a612e to 1313ee1 Compare July 14, 2018 16:02

jbrockmendel reviewed Jul 19, 2018

View reviewed changes

jreback requested changes Jul 20, 2018

View reviewed changes

elmq0022 force-pushed the remove_multiindex_pickles branch from 1313ee1 to 768adf2 Compare July 22, 2018 23:08

elmq0022 changed the title ~~CLN: remove legacy MultiIndex #21654~~ CLN: Remove Legacy MultiIndex Index Compatibility Jul 24, 2018

elmq0022 force-pushed the remove_multiindex_pickles branch from 768adf2 to 08a9701 Compare July 24, 2018 01:57

elmq0022 force-pushed the remove_multiindex_pickles branch from 08a9701 to aad8ccb Compare July 25, 2018 01:40

jreback requested changes Jul 25, 2018

View reviewed changes

jreback added this to the 0.24.0 milestone Jul 25, 2018

jorisvandenbossche requested changes Jul 25, 2018

View reviewed changes

fix issue pandas-dev#21654

ae446bc

elmq0022 force-pushed the remove_multiindex_pickles branch from aad8ccb to ae446bc Compare July 27, 2018 02:10

jorisvandenbossche approved these changes Jul 27, 2018

View reviewed changes

jorisvandenbossche merged commit 114f415 into pandas-dev:master Jul 27, 2018

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

CLN: Remove Legacy MultiIndex Index Compatibility (pandas-dev#21740)

598dce7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLN: Remove Legacy MultiIndex Index Compatibility #21740

CLN: Remove Legacy MultiIndex Index Compatibility #21740

elmq0022 commented Jul 5, 2018 •

edited

Loading

jreback left a comment

jreback Jul 5, 2018

elmq0022 Jul 6, 2018

codecov bot commented Jul 5, 2018 •

edited

Loading

elmq0022 commented Jul 5, 2018 via email

jreback Jul 6, 2018

jreback Jul 6, 2018

jreback Jul 6, 2018

elmq0022 commented Jul 14, 2018 •

edited

Loading

jbrockmendel Jul 19, 2018

jbrockmendel commented Jul 19, 2018

elmq0022 commented Jul 19, 2018 via email

jreback Jul 20, 2018

elmq0022 Jul 23, 2018

jorisvandenbossche Jul 23, 2018

elmq0022 commented Jul 24, 2018

elmq0022 commented Jul 25, 2018

jreback left a comment

jreback Jul 25, 2018

elmq0022 commented Jul 25, 2018 via email

jorisvandenbossche Jul 25, 2018

jreback Jul 25, 2018

jorisvandenbossche Jul 25, 2018

jreback Jul 25, 2018

jorisvandenbossche Jul 26, 2018

elmq0022 commented Jul 26, 2018 via email

elmq0022 commented Jul 27, 2018

jorisvandenbossche left a comment

elmq0022 commented Jul 27, 2018 via email

CLN: Remove Legacy MultiIndex Index Compatibility #21740

CLN: Remove Legacy MultiIndex Index Compatibility #21740

Conversation

elmq0022 commented Jul 5, 2018 • edited Loading

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

codecov bot commented Jul 5, 2018 • edited Loading

Codecov Report

elmq0022 commented Jul 5, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elmq0022 commented Jul 14, 2018 • edited Loading

Choose a reason for hiding this comment

jbrockmendel commented Jul 19, 2018

elmq0022 commented Jul 19, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elmq0022 commented Jul 24, 2018

elmq0022 commented Jul 25, 2018

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elmq0022 commented Jul 25, 2018 via email

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

elmq0022 commented Jul 26, 2018 via email

elmq0022 commented Jul 27, 2018

jorisvandenbossche left a comment

Choose a reason for hiding this comment

elmq0022 commented Jul 27, 2018 via email

elmq0022 commented Jul 5, 2018 •

edited

Loading

codecov bot commented Jul 5, 2018 •

edited

Loading

elmq0022 commented Jul 14, 2018 •

edited

Loading