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

henriqueribeiro
Copy link
Contributor

@henriqueribeiro henriqueribeiro 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
Copy link
Contributor

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:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right! I will change that

@codecov
Copy link

codecov bot 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
Copy link
Contributor Author

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

@jbrockmendel
Copy link
Member

Override for MI with plural names?

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Sep 4, 2018 via email

@gfyoung gfyoung added Enhancement Needs Discussion Requires discussion from core team before further action Compat pandas objects compatability with Numpy or Python functions labels Sep 5, 2018
@gfyoung
Copy link
Member

gfyoung 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
Copy link

pep8speaks commented Sep 6, 2018

Hello @henriqueribeiro! Thanks for updating the PR.

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

@henriqueribeiro
Copy link
Contributor Author

@gfyoung Sure! Sorry for that

@henriqueribeiro
Copy link
Contributor Author

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

@TomAugspurger
Copy link
Contributor

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

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 Show resolved Hide resolved
@TomAugspurger
Copy link
Contributor

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


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
Copy link
Contributor

Choose a reason for hiding this comment

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

'series' -> 'index'

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 Show resolved Hide resolved
@TomAugspurger
Copy link
Contributor

Needs a release note too, for 0.24

@henriqueribeiro
Copy link
Contributor Author

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

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Sep 11, 2018 via email

pandas/core/indexes/multi.py Outdated Show resolved Hide resolved
pandas/tests/indexes/common.py Outdated Show resolved Hide resolved
@jreback jreback added this to the 0.24.0 milestone Sep 13, 2018
@jreback
Copy link
Contributor

jreback commented Sep 13, 2018

lgtm. was there not a referenced issue?

@henriqueribeiro
Copy link
Contributor Author

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

@gfyoung
Copy link
Member

gfyoung 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
@gfyoung
Copy link
Member

gfyoung commented Sep 14, 2018

Thanks @henriqueribeiro !

@henriqueribeiro
Copy link
Contributor Author

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

aeltanawy pushed a commit to aeltanawy/pandas that referenced this pull request Sep 20, 2018
Sup3rGeo pushed 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
Labels
Compat pandas objects compatability with Numpy or Python functions Enhancement Needs Discussion Requires discussion from core team before further action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants