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

DEPR: MultiIndex.to_hierarchical #21613

Merged
merged 8 commits into from Jun 26, 2018
Merged

DEPR: MultiIndex.to_hierarchical #21613

merged 8 commits into from Jun 26, 2018

Conversation

@KalyanGokhale
Copy link
Contributor

@KalyanGokhale KalyanGokhale commented Jun 24, 2018

xref #18262

  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry
@codecov
Copy link

@codecov codecov bot commented Jun 24, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #21613      +/-   ##
==========================================
+ Coverage    91.9%   91.91%   +<.01%     
==========================================
  Files         153      153              
  Lines       49547    49534      -13     
==========================================
- Hits        45537    45527      -10     
+ Misses       4010     4007       -3
Flag Coverage Δ
#multiple 90.3% <100%> (ø) ⬆️
#single 41.87% <0%> (+0.08%) ⬆️
Impacted Files Coverage Δ
pandas/core/indexes/multi.py 94.89% <100%> (-0.08%) ⬇️
pandas/core/panel.py 97.87% <100%> (ø) ⬆️
pandas/core/indexes/category.py 97.01% <0%> (-0.28%) ⬇️
pandas/core/indexes/base.py 96.63% <0%> (ø) ⬆️
pandas/compat/__init__.py 58.6% <0%> (+0.17%) ⬆️
pandas/core/indexing.py 93.73% <0%> (+0.36%) ⬆️

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 f1aa08c...1d2eb26. Read the comment docs.

# GH21613
# .to_hierarchical will be deprecated
with tm.assert_produces_warning(FutureWarning):
result = index.to_hierarchical(2)
Copy link
Contributor

@jreback jreback Jun 25, 2018

Choose a reason for hiding this comment

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

you don't need the result =

Copy link
Contributor Author

@KalyanGokhale KalyanGokhale Jun 25, 2018

Choose a reason for hiding this comment

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

Thanks - yes had missed it - will update together along with other comments

@@ -1704,6 +1704,11 @@ def test_to_hierarchical(self):
tm.assert_index_equal(result, expected)
Copy link
Contributor

@jreback jreback Jun 25, 2018

Choose a reason for hiding this comment

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

you need to catch all uses of to_hierarchical (or you get the warnings)

Copy link
Contributor Author

@KalyanGokhale KalyanGokhale Jun 25, 2018

Choose a reason for hiding this comment

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

Thanks - not sure if I understand.
The only use of to_hierarchical was in pandas/core/panel.py- this has been substituted.
This existing test is specifically for to_hierarchical - which can be removed once this method is removed in a future version - should these tests be removed as part of this PR?

Copy link
Contributor Author

@KalyanGokhale KalyanGokhale Jun 25, 2018

Choose a reason for hiding this comment

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

@jreback I think after reviewing changes in #18258 I probably get what is expected - please ignore the comment above - will update

@@ -1182,6 +1182,8 @@ def to_frame(self, index=True):

def to_hierarchical(self, n_repeat, n_shuffle=1):
"""
Copy link
Contributor

@jreback jreback Jun 25, 2018

Choose a reason for hiding this comment

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

to_hierarchical is listed elsewhere in this file in a doc-string

Copy link
Contributor Author

@KalyanGokhale KalyanGokhale Jun 25, 2018

Choose a reason for hiding this comment

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

Thanks - it is mentioned as a part of enumeration of methods of MultiIndex - should it be removed?

Copy link
Contributor

@jreback jreback Jun 25, 2018

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

@KalyanGokhale KalyanGokhale Jun 26, 2018

Choose a reason for hiding this comment

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

Thanks - updated for all of the latest review comments

@@ -1182,6 +1182,8 @@ def to_frame(self, index=True):

def to_hierarchical(self, n_repeat, n_shuffle=1):
"""
Copy link
Contributor

@jreback jreback Jun 25, 2018

Choose a reason for hiding this comment

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

yes

@@ -948,10 +948,13 @@ def to_frame(self, filter_observations=True):
data[item] = self[item].values.ravel()[selector]

def construct_multi_parts(idx, n_repeat, n_shuffle=1):
axis_idx = idx.to_hierarchical(n_repeat, n_shuffle)
Copy link
Contributor

@jreback jreback Jun 25, 2018

Choose a reason for hiding this comment

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

can you provide a 1-liner what this method is doing

@@ -1675,15 +1675,19 @@ def test_to_frame(self):
def test_to_hierarchical(self):
index = MultiIndex.from_tuples([(1, 'one'), (1, 'two'), (2, 'one'), (
2, 'two')])
result = index.to_hierarchical(3)
# GH21613
# Suppressed deprecation warnings in this original test
Copy link
Contributor

@jreback jreback Jun 25, 2018

Choose a reason for hiding this comment

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

these comments are not needed, move the gh issue number to the top of this method

@jreback jreback added this to the 0.24.0 milestone Jun 26, 2018
@jreback jreback merged commit af8d174 into pandas-dev:master Jun 26, 2018
3 checks passed
@jreback
Copy link
Contributor

@jreback jreback commented Jun 26, 2018

thanks!

@jreback jreback mentioned this pull request Jun 26, 2018
34 tasks
@KalyanGokhale KalyanGokhale deleted the MItoHier branch Jun 26, 2018
Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this issue Oct 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants