fixed timedelta issue in longest_contiguous_slice #725
Conversation
dennisbader
requested review from
hrzn,
pennfranc and
tomasvanpottelbergh
as code owners
Codecov Report
@@ Coverage Diff @@
## master #725 +/- ##
=======================================
Coverage 90.80% 90.80%
=======================================
Files 67 67
Lines 6740 6740
=======================================
Hits 6120 6120
Misses 620 620
Continue to review full report at Codecov.
|
darts/timeseries.py
Outdated
| @@ -1468,7 +1468,7 @@ def longest_contiguous_slice(self, max_gap_size: int = 0) -> 'TimeSeries': | |||
| max_slice_start = None | |||
| max_slice_end = None | |||
| for index, row in relevant_gaps.iterrows(): | |||
| size = row['gap_start'] - curr_slice_start - self._freq | |||
There was a problem hiding this comment.
Does changing the order of the operands make a difference here?
There was a problem hiding this comment.
Yes,
row['gap_start'] - curr_slice_startgives a time delta in days from which we cannot subtract the frequency.row['gap_start'] - self._freqgives a time stamp from which we can subtractcurr_slice_start
There was a problem hiding this comment.
Could we change our API to avoid relying on the order of the operands? Because this seems a bit fragile... I would expect a subtraction to always return the same type.
There was a problem hiding this comment.
This is how pandas handles the subtraction
pd.Timestamp - pd.Timestamp -> pd.TimeDeltapd.Timestamp - pandas DateOffset (freq) -> pd.Timestamp
As TimeDelta doesn't carry information about the exact dates, we cannot subtract irregular frequencies (such as month start 'MS', etc.) which is why the original operand order fails.
For regular frequencies (such as 'D') the order doesn't matter for the results.
I don't believe it is possible to account for this in our API (as we deal with dates) but maybe I'm missing something.
Maybe we could make it more obvious with:
curr_slice_end = row['gap_start'] - self._freq
size = curr_slice_end - curr_slice_start
?
There was a problem hiding this comment.
+1 with your solution (and adding a comment) @dennisbader
…t8co/darts into fix/longest_contiguous_slice
* fixed timedelta issue in longest_contiguous_slice * made slice size calculation more comprehensible
Fixes #716
Summary
TimeSeries.longest_contiguous_slice()