Remove keep_tz kwarg from DatetimeIndex.to_frame #17826
Conversation
| @@ -915,46 +915,6 @@ def to_series(self, keep_tz=False): | |||
| index=self._shallow_copy(), | |||
| name=self.name) | |||
|
|
|||
| def to_frame(self, index=True, keep_tz=False): | |||
| """ | |||
There was a problem hiding this comment.
If the keep_tz keyword is removed, then yes, this should be duplicate code.
See #17815 (comment) on removing that keyword or not (you merged the PR before my objection was dismissed or adapted :-))
There was a problem hiding this comment.
hmm do we need to deprecate?
There was a problem hiding this comment.
nope, because you only merged it a few hours ago (@gfyoung was adding a new method).
(we might consider deprecating it for Index.to_series, but that is another issue.
There was a problem hiding this comment.
Not really sure I agree with the choice to remove this argument. It could be confusing to people as to why we support it in one method (to_series) but not the other. Also, your argument as a historical argument was not particularly convincing since you were not 100% sure yourself.
There was a problem hiding this comment.
If you think that keep_tz should not be a valid argument, then let's deprecate across the board. I would be okay with that.
There was a problem hiding this comment.
To illustrate my 'suboptimal behaviour' of above (this is current master):
In [1]: dtidx = pd.date_range("2017-01-01", periods=3, tz='Europe/Brussels')
In [2]: dtidx
Out[2]:
DatetimeIndex(['2017-01-01 00:00:00+01:00', '2017-01-02 00:00:00+01:00',
'2017-01-03 00:00:00+01:00'],
dtype='datetime64[ns, Europe/Brussels]', freq='D')
In [3]: dtidx.to_frame()
Out[3]:
0
2017-01-01 00:00:00+01:00 2016-12-31 23:00:00
2017-01-02 00:00:00+01:00 2017-01-01 23:00:00
2017-01-03 00:00:00+01:00 2017-01-02 23:00:00
In [5]: dtidx.to_frame().dtypes
Out[5]:
0 datetime64[ns]
dtype: object
so it was a timezone aware index, but it lost its timezone information in conversion to a dataframe.
There was a problem hiding this comment.
yeah I think we can deprecate the existing keep_tz and just always keep it. IIRC I put this in place before we had first class tz support and this was performance (hazy memory though).
@gfyoung do you or @jorisvandenbossche want to add deprecation onto this PR?
There was a problem hiding this comment.
I probably won't have time today, so will already merge this one.
I might have time later this week, but that will depend on when the RC is cut.
There was a problem hiding this comment.
Opened an issue for the other one: #17832
Codecov Report
@@ Coverage Diff @@
## master #17826 +/- ##
==========================================
- Coverage 91.24% 91.22% -0.02%
==========================================
Files 163 163
Lines 49992 49986 -6
==========================================
- Hits 45613 45598 -15
- Misses 4379 4388 +9
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #17826 +/- ##
==========================================
- Coverage 91.24% 91.22% -0.02%
==========================================
Files 163 163
Lines 49992 49986 -6
==========================================
- Hits 45613 45598 -15
- Misses 4379 4388 +9
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #17826 +/- ##
==========================================
- Coverage 91.24% 91.22% -0.02%
==========================================
Files 163 163
Lines 49992 49986 -6
==========================================
- Hits 45613 45598 -15
- Misses 4379 4388 +9
Continue to review full report at Codecov.
|
xref #17815 (comment)