BUG: Series.combine_first with mixed timezones #26379
Conversation
mroeschke
added
Bug
Reshaping
Codecov Report
@@ Coverage Diff @@
## master #26379 +/- ##
==========================================
- Coverage 91.68% 91.67% -0.01%
==========================================
Files 174 174
Lines 50704 50706 +2
==========================================
- Hits 46489 46487 -2
- Misses 4215 4219 +4
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #26379 +/- ##
==========================================
- Coverage 91.68% 91.67% -0.01%
==========================================
Files 174 174
Lines 50704 50706 +2
==========================================
- Hits 46489 46487 -2
- Misses 4215 4219 +4
Continue to review full report at Codecov.
|
| @@ -489,6 +489,9 @@ def union(self, other, sort=None): | |||
| other = DatetimeIndex(other) | |||
| except TypeError: | |||
| pass | |||
| except ValueError: | |||
| # GH 26283: Convert indexes with mixed tz to UTC | |||
| other = tools.to_datetime(other, utc=True) | |||
There was a problem hiding this comment.
this is not correct; rather you should call the super.union() as these must be combined as Index rather than DTI
There was a problem hiding this comment.
So super().union actually returns the same result because these evaluate as equal:
>>> Timestamp('2019-05-01 00:00:00', tz='UTC') == Timestamp('2019-05-01 01:00:00', tz='Europe/London')
True
While an Index with all Timestamps feels a little more correct, a DTI with UTC timestamps is technically correct too since the above is True. (And less code to handle this special casing is a nice plus)
There was a problem hiding this comment.
I disagree, we are striving never to coerce to UTC unless its explicit, this should stay as object dtyped.
|
This fix is a littler tricker than expected. Closing for now. |
git diff upstream/master -u -- "*.py" | flake8 --diff