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

Add 'name' as argument for index 'to_frame' method #22580

Merged

Conversation

Projects
None yet
6 participants
@henriqueribeiro
Copy link
Contributor

commented Sep 3, 2018

  • closes #xxxx
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

Since series to_frame method has a name argument, I believe it makes sense for index also have it.

@TomAugspurger

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2018

The problem is that a name argument doesn’t make sense for a MultiIndex. Not sure what’s best here.

"""

from pandas import DataFrame
name = self.name or 0
if not name:

This comment has been minimized.

Copy link
@TomAugspurger

TomAugspurger Sep 3, 2018

Contributor

This would have to be if name is not None, to allow falsey names.

This comment has been minimized.

Copy link
@henriqueribeiro

henriqueribeiro Sep 3, 2018

Author Contributor

You are right! I will change that

@codecov

This comment has been minimized.

Copy link

commented Sep 3, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #22580      +/-   ##
==========================================
+ Coverage   92.17%   92.17%   +<.01%     
==========================================
  Files         169      169              
  Lines       50708    50716       +8     
==========================================
+ Hits        46740    46748       +8     
  Misses       3968     3968
Flag Coverage Δ
#multiple 90.58% <100%> (ø) ⬆️
#single 42.34% <18.18%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/multi.py 95.43% <100%> (+0.02%) ⬆️
pandas/core/indexes/base.py 96.45% <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 b23a0e8...db54f2a. Read the comment docs.

@henriqueribeiro

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2018

@TomAugspurger regarding MultiIndex, I'm not seeing how this name will affect MultiIndex. Can you enlight me please?

@jbrockmendel

This comment has been minimized.

Copy link
Member

commented Sep 3, 2018

Override for MI with plural names?

@TomAugspurger

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2018

@gfyoung

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

@henriqueribeiro : Thanks for the contribution! In the future, if you could open an issue before opening the PR, that would allow us to evaluate the idea first before implementing.

@pep8speaks

This comment has been minimized.

Copy link

commented Sep 6, 2018

Hello @henriqueribeiro! Thanks for updating the PR.

Comment last updated on September 08, 2018 at 15:33 Hours UTC
@henriqueribeiro

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2018

@gfyoung Sure! Sorry for that

@henriqueribeiro

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2018

@TomAugspurger I cannot understand why some checks are failing. Can you give me some help please?

@TomAugspurger

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2018

There was a new release of openpyxl that's incompatible with pandas. It's been worked around on master, so you need to fetch upstream and push your changes.

git fetch upstream
git merge upstream/master
git push origin
Show resolved Hide resolved pandas/core/indexes/base.py Outdated
Show resolved Hide resolved pandas/core/indexes/multi.py Outdated
Show resolved Hide resolved pandas/core/indexes/multi.py Outdated
Show resolved Hide resolved pandas/core/indexes/multi.py Outdated
Show resolved Hide resolved pandas/tests/indexes/common.py Outdated
Show resolved Hide resolved pandas/tests/indexes/multi/test_conversion.py Outdated
Show resolved Hide resolved pandas/tests/indexes/multi/test_conversion.py Outdated
@TomAugspurger

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2018

@henriqueribeiro linting error https://travis-ci.org/pandas-dev/pandas/jobs/425773293#L3653 and it looks like you may need to merge master still.

henriqueribeiro added some commits Sep 8, 2018

Parameters
----------
index : boolean, default True
Set the index of the returned DataFrame as the original Index.
name : object, default None
The passed name should substitute for the series name (if it has

This comment has been minimized.

Copy link
@TomAugspurger

TomAugspurger Sep 10, 2018

Contributor

'series' -> 'index'

Show resolved Hide resolved pandas/core/indexes/base.py
Show resolved Hide resolved pandas/core/indexes/multi.py Outdated
Show resolved Hide resolved pandas/core/indexes/multi.py Outdated
Show resolved Hide resolved pandas/core/indexes/multi.py Outdated
Show resolved Hide resolved pandas/core/indexes/multi.py Outdated
@TomAugspurger

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2018

Needs a release note too, for 0.24

@henriqueribeiro

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2018

@TomAugspurger Regarding the release notes, should I add this on the indexing section of v0.24.0.txt?

@TomAugspurger

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

henriqueribeiro added some commits Sep 12, 2018

Show resolved Hide resolved pandas/core/indexes/multi.py Outdated
Show resolved Hide resolved pandas/tests/indexes/common.py Outdated

@jreback jreback added this to the 0.24.0 milestone Sep 13, 2018

@jreback

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2018

lgtm. was there not a referenced issue?

@henriqueribeiro

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2018

No. This came from dask and that's why I created the pull request immediately.

@gfyoung

This comment has been minimized.

Copy link
Member

commented Sep 14, 2018

All green, and since @jreback has already approved this, merging.

@gfyoung gfyoung merged commit 08bd3e3 into pandas-dev:master Sep 14, 2018

6 checks passed

ci/circleci: py27_compat Your tests passed on CircleCI!
Details
ci/circleci: py35_ascii Your tests passed on CircleCI!
Details
ci/circleci: py36_locale Your tests passed on CircleCI!
Details
ci/circleci: py36_locale_slow Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gfyoung

This comment has been minimized.

Copy link
Member

commented Sep 14, 2018

Thanks @henriqueribeiro !

@henriqueribeiro

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2018

Thanks to @TomAugspurger for this "ping-pong" fixing.

aeltanawy pushed a commit to aeltanawy/pandas that referenced this pull request Sep 20, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.