API: Tick arithmetic respects calendar time #22195
Conversation
|
|
||
| except NotImplementedError: | ||
| warnings.warn("Non-vectorized DateOffset being applied to Series " | ||
| "or DatetimeIndex", PerformanceWarning) | ||
| result = self.astype('O') + offset | ||
|
|
||
| return type(self)(result, freq='infer') | ||
| return type(self)(result, freq='infer', tz=self.tz) |
There was a problem hiding this comment.
This was changed because if the code goes down the NotImplementedError path, the tz would be dropped.
| DatetimeIndex | ||
| """ | ||
| # TODO: Add a vectorized DatetimeIndex.dst() method | ||
| ambiguous = np.array([bool(ts.dst()) if ts is not tslibs.NaT else False |
There was a problem hiding this comment.
I could delay this ambiguous calculation until an AmbiguousTimeError is raised?
|
Definitely +1 on doing this for Day, but it is less clear what this means for the other Tick subclasses. What's the motivation? |
|
For the other Ticks, it's mostly to keep them consistent with the definition of |
I can see some usefulness here, but what about use cases where the frequency should be Timedelta-like? e.g. if I call This is also going to affect |
| nanos = delta_to_nanoseconds(other) | ||
| result = Timestamp(self.value + nanos, | ||
| tz=self.tzinfo, freq=self.freq) | ||
| return result | ||
|
|
||
| elif hasattr(other, 'delta'): |
There was a problem hiding this comment.
You might be able to import tslibs.offsets._Tick here and make this an isinstance check (thought that might cause a circular import, not sure off the top)
|
I think the core confusion for users is the inherent ambiguity when specifying a string frequency like Pandas advertises them as (Date)offsets, but there's an implicit (and not-clearly-documented) consistency throughout the API that string offsets passed to other methods like That being said, I would love for all the frequency strings to map to calendar-respecting For the sake of consistency, I am okay with the API break, but interested to get your initial thoughts |
Codecov Report
@@ Coverage Diff @@
## master #22195 +/- ##
==========================================
+ Coverage 92.06% 92.06% +<.01%
==========================================
Files 169 169
Lines 50694 50697 +3
==========================================
+ Hits 46671 46674 +3
Misses 4023 4023
Continue to review full report at Codecov.
|
|
@jbrockmendel After thinking about it; I am convinced to only change To do this I was thinking of having |
|
@mroeschke ok with your suggestion, it might make some code simpler (though likely to break some tests I think) |
|
I think we're on the same page. I think we should make We also need to retain a way to specify There is also an issue (not specific to this PR, but closely related) of how to handle ambiguous/nonexistent times. If I start at Conversely, If I start at |
|
Thanks for the input @jreback and @jbrockmendel: In regards to the |
|
Closing as this will require a different implimentation |
git diff upstream/master -u -- "*.py" | flake8 --diffThis PR changes
Tickarithmetic to respect calendar time akin toDatoffsetforTimestampandDatetimIndex(and henceSeries). There may be some embedded Tick work-around code (I am thinking inDatetimeIndex._generateandresamplethat may be refactorable after this change.