Numindexname by toobaz · Pull Request #13205 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Closed
Numindexname #13205
Conversation102 Commits6 Checks0 Files changed
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.
| expected = Index(i.values.astype(dtype)) |
|---|
| tm.assert_index_equal(result, expected) |
| i = Float64Index([0, 1, 2]) |
| result = i.astype('int64') |
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
| if name is None and hasattr(levels, 'name'): |
|---|
| name = levels.name |
| if isinstance(levels, MultiIndex): |
| return levels.copy(name=name, names=names, deep=copy) |
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
| if copy or data.dtype != cls._default_dtype: |
|---|
| subarr = np.array(data, dtype=cls._default_dtype, copy=copy) |
| if not ((subarr == data) | np.isnan(subarr)).all(): |
| raise TypeError('Unsafe NumPy casting, you must ' |
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.
| @@ -82,8 +84,6 @@ def __new__(cls, levels=None, labels=None, sortorder=None, names=None, |
|---|
| if len(levels) == 1: |
| if names: |
| name = names[0] |
| else: |
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)
| def test_ensure_copied_data(self): |
|---|
| # GH12309 |
| for name, index in compat.iteritems(self.indices): |
| if isinstance(index, (PeriodIndex, RangeIndex, MultiIndex)): |
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)
| ---------- |
|---|
| orig : ndarray |
| other ndarray to compare self._data against |
| copy : boolean |
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.
| with assertRaisesRegexp(AssertionError, expected): |
|---|
| assert_numpy_array_equal(a, b, check_same='same') |
| expected = 'array\(\[1, 2, 3\]\) is array\(\[1, 2, 3\]\)' |
| with assertRaisesRegexp(AssertionError, expected): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good
| @@ -343,7 +343,8 @@ Bug Fixes |
|---|
| - Bug in ``.tz_convert`` on a tz-aware ``DateTimeIndex`` that relied on index being sorted for correct results (:issue: `13306`) |
| - Bug in various index types, which did not propagate the name of passed index (:issue:`12309`) |
| - Bug in ``DateTimeIndex``, which did not honour the ``copy=True`` (:issue:`13205`) |
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