Numindexname by toobaz · Pull Request #13205 · pandas-dev/pandas (original) (raw)
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- closes Int64Index and Float64Index (and... ?) do not propagate name of passed index #12309
- tests added / passed
- passes
git diff upstream/master | flake8 --diff - whatsnew entries: name passing, unsupported
dtypes,copy=trueforDateTimeIndex
This is a rebased & updated version of #12331
I would say the last commit is the most critical one: it exposes the consequences of making numeric indices only support their default dtype. By the way: I had interpreted your comment as "raise if dtype is set", but I don't think this is what we want, since it breaks the obj -> str(obj) -> obj roundtrip. So I raise only if dtype is not the expected dtype.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can't change this. a float32 should simply be promoted to float64 (and likewise int32 to int64)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(right, fixed)
Related to indexing on series/frames, not to indexes themselves
pandas objects compatability with Numpy or Python functions
labels
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this needed for? passing a levels multi-index is very odd
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed we want idx.__class__(idx) to return a copy of idx for any index idx. But maybe it was just my fantasy...
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well did you test this? if this were to be supported then it should be the very first check.
I am not averse (and other Indexes do this), but needs testing.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is tested here
Current coverage is 84.25%
@@ master #13205 diff @@
Files 138 138
Lines 50805 50810 +5
Methods 0 0
Messages 0 0
Branches 0 0
- Hits 42796 42810 +14
- Misses 8009 8000 -9
Partials 0 0
Powered by Codecov. Last updated by 62b4327...9d93fea
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is extremely expensive to compare things like this,why are you doingit?
Removed dtype check and the related tests. Sorry for filling up the Travis queue... three of the tests can be canceled.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are you removing this? names/name are equivalent (I think this is taken care above), pls confirm.
(removed code for name handling in MultiIndex, which is now special-cased in the relative test)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so then these need specific tests (name this the same as this one and put in the appropriate place)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make this a default parameter of False.
Added the new tests, will have to update a couple of calls when we decide what to do in #13355, but that can be done there, and I wouldn't wait (at least, I personally don't want to spend more time on this PR).
#13355 can fix it if desired (but to be honest that is lessening the strictness of tests), but can decide there.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DatetimeIndex
Thx for update. Few minor comments.
thanks @toobaz the internal refactorings can take some time. great for sticking with it!
Hope I've learned a bit that will help me need less passages next time ;-)
Labels
pandas objects compatability with Numpy or Python functions
Related to indexing on series/frames, not to indexes themselves