BUG: concat/append misc fixes #13660
Conversation
sinhrks
added
Bug
Reshaping
| return to_concat, name | ||
| if len(_concat.get_dtype_kinds(to_concat)) > 1: | ||
| # ToDo: Use ABC | ||
| from pandas.tseries.api import (DatetimeIndex, PeriodIndex, |
There was a problem hiding this comment.
maybe move this to types/concat (e.g. its a concat_compat type of thing)
|
whoosh! you really cleaned this up! had a quick look, but will look deeper later. |
Current coverage is 85.26% (diff: 97.05%)@@ master #13660 diff @@
==========================================
Files 139 139
Lines 50562 50506 -56
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 43116 43066 -50
+ Misses 7446 7440 -6
Partials 0 0
|
sinhrks
force-pushed
the
concat_cln
branch
6 times, most recently
from
87d9ad9 to
92746ca
Compare
sinhrks
force-pushed
the
concat_cln
branch
2 times, most recently
from
2f901ba to
debce91
Compare
|
status? |
| - Bug in ``PeriodIndex.append`` may raises ``AttributeError`` when the result is ``object`` dtype (:issue:`13221`) | ||
| - Bug in ``CategoricalIndex.append`` may accept normal ``list`` (:issue:`13626`) | ||
| - Bug in ``pd.concat`` and ``.append`` with the same timezone get reset to UTC (:issue:`7795`) | ||
| - Bug in ``Series`` and ``DataFrame`` ``.append`` raises ``AmbiguousTimeError`` if data contains datetime near DST boundary (:issue:`14626`) |
There was a problem hiding this comment.
this issue number is wrong
|
you seem to fix a pivot name propogation issue. is there an issue? |
sinhrks
force-pushed
the
concat_cln
branch
2 times, most recently
from
b12a13b to
911fb66
Compare
|
you must have just rebased this! will look tom. |
|
Yeah let me work on this first, I'll include the cleanup for Let you know once ready. |
sinhrks
changed the title
|
@jreback updated, and ready for review. |
| all inputs must be DatetimeIndex | ||
| it is used in DatetimeIndex.append also | ||
| """ | ||
| # no need to localize because internal repr will not be changed |
There was a problem hiding this comment.
I think we should do a run-time assert that these meet the guarantees
e.g.
assert len(set([ x.tz for x in to_concat ])) == 1
|
@sinhrks looks really nice. just some minor comments and guaranetees that should specify a bit more / use asserts. ping on green. |
| name = None | ||
| break | ||
| if not isinstance(obj, Index): | ||
| raise TypeError('all inputs must be Index') |
There was a problem hiding this comment.
(a bit similar with the equals case)
Currently, we are accepting a list or tuple to be added (in case that it is a list of list, in the flat case you get an error):
In [56]: idx = pd.Index([1,2])
In [57]: idx.append([[1,2]])
Out[57]: Int64Index([1, 2, 1, 2], dtype='int64')
The docstring of append also says for other: "Index or list/tuple of indices"
There was a problem hiding this comment.
_ensure_index should handle this (it might think its a MultiIndex actually), so might need to disambiguate
There was a problem hiding this comment.
yes, _ensure_index would convert list to Index. @jreback are you saying Index.append should accept lists?
On second thought, maybe I misread the Index.append docstring. The "Index or list/tuple of indices" can be interpreted as both as "list/tuple of Index objects" (so no guarantee to accept list as Index object) or "list of index labels".
But actually probably the first interpretation. In that case, I think this PR is OK, but @sinhrks it would maybe be good to list this in the whatsnew as change as well? (or do we regard it as a bug fix?)
There was a problem hiding this comment.
Yes, I agree docstring intends first one. I'm adding an validation to check all elements are Index if input is list-like.
I think it is regarded as a bug fix, because it is similar to the fix which CategiricalIndex.append accepts (flat) list of labels (included in this PR).
sinhrks
force-pushed
the
concat_cln
branch
2 times, most recently
from
2fabfcd to
b17d8dc
Compare
closes #13626
closes #7795