Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Revert "MAINT: Remove Long and WidePanel (#15748)" (#15802)" #18341

Merged
merged 1 commit into from
May 16, 2018

Conversation

gfyoung
Copy link
Member

@gfyoung gfyoung commented Nov 17, 2017

With statsmodels/statsmodels#3580 now resolved, we can safely revert #15802

@gfyoung gfyoung added Clean Deprecate Functionality to remove in pandas Panel labels Nov 17, 2017
@gfyoung gfyoung added this to the 0.22.0 milestone Nov 17, 2017
@codecov
Copy link

codecov bot commented Nov 17, 2017

Codecov Report

Merging #18341 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18341      +/-   ##
==========================================
+ Coverage   91.82%   91.83%   +<.01%     
==========================================
  Files         153      153              
  Lines       49503    49495       -8     
==========================================
- Hits        45458    45454       -4     
+ Misses       4045     4041       -4
Flag Coverage Δ
#multiple 90.23% <100%> (ø) ⬆️
#single 41.88% <100%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/panel.py 97.87% <ø> (+0.57%) ⬆️
pandas/core/api.py 100% <100%> (ø) ⬆️

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 27581e9...854be64. Read the comment docs.

@jorisvandenbossche
Copy link
Member

Let's still wait until there is an actual statsmodels release. At this point we don't know yet who of statsmodels or pandas will release first.

@jreback
Copy link
Contributor

jreback commented Nov 17, 2017

going to merge #18333 which will causes this to fail. We weren't fully testing things in the downstream.

@jreback
Copy link
Contributor

jreback commented Nov 17, 2017

@gfyoung go ahead and rebase. This should fail. We can decide whether to xfail the test at a later point.

@gfyoung
Copy link
Member Author

gfyoung commented Nov 17, 2017

Let's still wait until there is an actual statsmodels release. At this point we don't know yet who of statsmodels or pandas will release first.

@jorisvandenbossche : That's unfortunate, as they said that it would come out by end of summer. Not sure what happened there.

We can decide whether to xfail the test at a later point.

@jreback : Not sure what "the test" is, but yep, will do.

@gfyoung
Copy link
Member Author

gfyoung commented Nov 17, 2017

Ummm...@jreback : what test was supposed to fail? 😄

@jorisvandenbossche
Copy link
Member

@jorisvandenbossche : That's unfortunate, as they said that it would come out by end of summer. Not sure what happened there.

Well, that's the reality of volunteer-based open source. It's indeed a bit annoying for us to have to keep it, but also not that costly.
We can just leave the PR open until the new statsmodels is out I would say.

@gfyoung
Copy link
Member Author

gfyoung commented Nov 18, 2017

Well, that's the reality of volunteer-based open source. It's indeed a bit annoying for us to have to keep it, but also not that costly.

Yeah...things happen. I have no problems waiting.

@bashtage
Copy link
Contributor

Let's still wait until there is an actual statsmodels release. At this point we don't know yet who of statsmodels or pandas will release first.

IMO pandas should make this change since it provides incentive for statsmodels to drop a release.

@jreback
Copy link
Contributor

jreback commented Nov 19, 2017

@gfyoung once I merge this, it should fail: #18362

turns out was skipping the tests :<

@jreback
Copy link
Contributor

jreback commented Nov 19, 2017

Then I agree with @bashtage we should xfail this test and merge. but let it fail to prove first.

@gfyoung
Copy link
Member Author

gfyoung commented Nov 19, 2017

IMO pandas should make this change since it provides incentive for statsmodels to drop a release.

@bashtage : I actually hadn't thought of that. Also makes sense.

Then I agree with @bashtage we should xfail this test and merge. but let it fail to prove first.

Sounds good to me.

@jreback
Copy link
Contributor

jreback commented Nov 19, 2017

@gfyoung rebase and let's see

@gfyoung
Copy link
Member Author

gfyoung commented Nov 19, 2017

@jreback : Can confirm that Travis is failing as a result of your PR. I will @xfail the test once Circle and AppVeyor have finished.

@gfyoung
Copy link
Member Author

gfyoung commented Nov 19, 2017

@jreback : And adding @xfail does fix everything. All is green again.

@@ -2155,8 +2147,8 @@ def test_multiindex_get(self):
assert (f1.items == [1, 2]).all()
assert (f2.items == [1, 2]).all()

ind = MultiIndex.from_tuples([('a', 1), ('a', 2), ('b', 1)],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this last section compare?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not entirely sure? I just saw this variable being assigned and not being used afterwards.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, if you can remove that (doesn't look like it does anything) would be great.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. Done.

@jreback
Copy link
Contributor

jreback commented Nov 19, 2017

@TomAugspurger is there a statsmodel 0.9.0 release issue? (where we can reference this)

@gfyoung
Copy link
Member Author

gfyoung commented Nov 19, 2017

@jreback : Judging from the milestone search here, statsmodels/statsmodels#3431 might be it? The issue is a little sparse though.

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Nov 19, 2017 via email

@jorisvandenbossche
Copy link
Member

Still -1 on merging this. I would say let's try to convince them with words to do a release instead of by breaking their code.

@jreback
Copy link
Contributor

jreback commented Nov 19, 2017

ok

@gfyoung let's remove the xfail and just leave this PR until/when statsmodels can do a release.

@gfyoung gfyoung force-pushed the long-wide-panel branch 4 times, most recently from 36b4866 to b252049 Compare April 30, 2018 00:37
@jreback jreback mentioned this pull request Apr 30, 2018
71 tasks
@gfyoung gfyoung force-pushed the long-wide-panel branch 5 times, most recently from c34fe91 to 4e1810a Compare May 6, 2018 08:23
@gfyoung gfyoung force-pushed the long-wide-panel branch 7 times, most recently from 3e5f43c to 9bb6c82 Compare May 14, 2018 04:04
@gfyoung gfyoung force-pushed the long-wide-panel branch 2 times, most recently from 77618bd to 83ce112 Compare May 16, 2018 10:00
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gfyoung I think can finally merge this as sm 0.9.0 is out.

@@ -1079,6 +1079,8 @@ Removal of prior version deprecations/changes
so operations would continue to work. This is now fully removed, so a ``Resampler`` will no longer forward compat operations (:issue:`20554`)
- Remove long deprecated ``axis=None`` parameter from ``.replace()`` (:issue:`20271`)

- The ``LongPanel`` and ``WidePanel`` classes have been removed (:issue:`10892`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to 0.24

Copy link
Member Author

@gfyoung gfyoung May 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, will do. Finally! 😄

@gfyoung gfyoung force-pushed the long-wide-panel branch 2 times, most recently from 28edc49 to 854be64 Compare May 16, 2018 11:57
@gfyoung
Copy link
Member Author

gfyoung commented May 16, 2018

@jreback : All is green here.

@jorisvandenbossche jorisvandenbossche merged commit 501f041 into pandas-dev:master May 16, 2018
@jorisvandenbossche
Copy link
Member

Thanks!

@gfyoung
Copy link
Member Author

gfyoung commented May 16, 2018

Sure thing. Hopefully we won't need to revert this revert 😄

@gfyoung gfyoung deleted the long-wide-panel branch May 16, 2018 14:15
@topper-123
Copy link
Contributor

The deprecating issue was #10892

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Deprecate Functionality to remove in pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants