-
-
Couldn't load subscription status.
- Fork 19.2k
BUG: Index does not inherit existing Index or DatatetimeIndex object … #11695
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
Conversation
|
Great!
I wonder if you could just call |
|
I'll rebase on master and squash my commits. I should update my PR shortly. |
doc/source/whatsnew/v0.18.0.txt
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just say creation, don't use inheriting, which has a different meaning
doc/source/whatsnew/v0.18.0.txt
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use double backticks around Index and DatetimeIndex
|
minor correction, pls rebase as I just merged #11497 which might impact this. ping when green. |
|
I've just rebased @jreback I'll add a second comment when the Travis CI build is completed. |
pandas/tests/test_index.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just construct the expected index and compare
expected = self.create_index()
expected.name = 'foo'
tm.assert_index_equal(result, expected)
also check with a new passed name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can work on this. Please ignore the last PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jreback What about the case when the object passed to Index is a MultiIndex? I think this is a corner case and should be avoided in principle. As a workaround, I would write the unit test as follows, even though is not very elegant:
expected = self.create_index()
if not isinstance(expected, MultiIndex):
expected.name = 'foo'
result = pd.Index(expected)
tm.assert_index_equal(result, expected)
result = pd.Index(expected, name='bar')
expected.name = 'bar'
tm.assert_index_equal(result, expected)
Should Index raise an error when we try to pass a MultiIndex object?
|
@jreback let me know if this needs further changes. |
doc/source/whatsnew/v0.18.0.txt
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need a doc-note, and you are changing things which are not relevant. e.g. pls rebase on master first.
|
@IamGianluca can you update |
…ame, when a new name is not provided fix missing lines in whatsnew cosmetic change in whatsnew refactor unit test, failing for MultiIndex short sentence in whatsnew doc fix unit test remove commented code fix typo add doc string add unit test for multiindex case
|
I've updated the unit test @jreback |
|
merged via fc40dcc thanks! |
This PR is meant to fix issue #11193
The issue prevent, when creating a new
Indexobject based on a passedIndexorDatetimeIndexobject, to inherit the existingIndexname if a new name is not provided.I've also created two unit test which test the new behaviour on both cases (passing an
IndexorDatetimeIndexobject) and added a comment in the whatsnew documentation.