CLN: de-duplicate generate_range #23218
Conversation
Codecov Report
@@ Coverage Diff @@
## master #23218 +/- ##
==========================================
+ Coverage 92.19% 92.2% +<.01%
==========================================
Files 169 169
Lines 50972 50968 -4
==========================================
- Hits 46996 46993 -3
+ Misses 3976 3975 -1
Continue to review full report at Codecov.
|
pandas/core/arrays/datetimelike.py
Outdated
| @@ -332,6 +332,11 @@ def _validate_frequency(cls, index, freq, **kwargs): | |||
| # Frequency validation is not meaningful for Period Array/Index | |||
| return None | |||
|
|
|||
| # DatetimeArray may pass `ambiguous`, nothing else will be accepted | |||
| # by cls._generate_range below | |||
There was a problem hiding this comment.
I don't really like this, can we just directly pass ambiguous as required?
There was a problem hiding this comment.
We could, but it is much more verbose (BTW the letter "v" on my keyboard is super-sticky, so we really need to stop talking about erbosity).
This would end up looking something like:
if is_timedelta64_dtype(index):
assert ambiguous is None
on_freq = cls._generate_range(start=index[0], end=None,
periods=len(index), freq=freq)
else:
on_freq = cls._generate_range(start=index[0], end=None,
periods=len(index), freq=freq, ambiguous=ambiguous)
Or do you have something else in mind? I like this less than the way the PR currently does it, but by a small enough margin that I'm fine changing it to get this done.
There was a problem hiding this comment.
no what I would do is add this to DatetimeIndex. It doesn't totally de-duplicate but is more correct and less hacky here
@classmethod
def _generate_range(cls, start, end, periods, name=None, freq=None,
tz=None, normalize=False, ambiguous='raise',
closed=None):
return super(DatetimeIndex, cls)._generate_range(
start, end, periods, freq,
tz=tz, normalize=normalize, ambiguous=ambiguous, closed=closed)
pandas/core/arrays/datetimelike.py
Outdated
| @@ -332,6 +332,11 @@ def _validate_frequency(cls, index, freq, **kwargs): | |||
| # Frequency validation is not meaningful for Period Array/Index | |||
| return None | |||
|
|
|||
| # DatetimeArray may pass `ambiguous`, nothing else will be accepted | |||
| # by cls._generate_range below | |||
There was a problem hiding this comment.
no what I would do is add this to DatetimeIndex. It doesn't totally de-duplicate but is more correct and less hacky here
@classmethod
def _generate_range(cls, start, end, periods, name=None, freq=None,
tz=None, normalize=False, ambiguous='raise',
closed=None):
return super(DatetimeIndex, cls)._generate_range(
start, end, periods, freq,
tz=tz, normalize=normalize, ambiguous=ambiguous, closed=closed)
Um, I don't see how that is really relevant to the |
|
I mean, I could just take out the new assertion in |
jorisvandenbossche
changed the title
then let's just make this the default. This is a very kludgy way of doing that. |
|
Hello @jbrockmendel! Thanks for updating the PR.
|
| @@ -703,6 +703,12 @@ def astype(self, dtype, copy=True): | |||
| raise TypeError(msg.format(name=type(self).__name__, dtype=dtype)) | |||
| return super(DatetimeIndexOpsMixin, self).astype(dtype, copy=copy) | |||
|
|
|||
| @Appender(DatetimeLikeArrayMixin._time_shift.__doc__) | |||
| def _time_shift(self, periods, freq=None): | |||
There was a problem hiding this comment.
how is this related to the rest of the PR?
There was a problem hiding this comment.
This PR changes DatetimeArrayMixin._time_shift such that it no longer pins self.name for Index subclasses.
|
looks fine. not super happy about having to set name all over the place, but this is an improvement over before. merging after CI is fixed. |
|
thanks! |
This reverts commit 225b9b0.
Split off from #23140.