New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix/ts gaps #1265
Fix/ts gaps #1265
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
@@ -1111,13 +1116,20 @@ def test_gaps(self): | |||
) | |||
).all() | |||
) | |||
gaps7 = series7.gaps() | |||
self.assertTrue(gaps7.empty) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, thanks for these tests!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only have some comments related to rendering of the docstrings. Otherwise it looks great, thanks @madtoinou !
darts/timeseries.py
Outdated
if self._has_datetime_index: | ||
return pd.date_range(start=start, end=end, freq=self._freq).size | ||
else: | ||
return end - start + 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually we do need to keep the division by self.freq here. For integer-indexed series, self.freq represents the "gap" between any two consecutive timestamps. For instance it's possible for an integer indexed series to have timestamps 0, 2, 4, ..., in which case freq=2. If we intend for the length to represent the number of NaN time steps, then we should keep the division by the freq.
""" | ||
Return the largest TimeSeries slice of this deterministic series that contains no gaps | ||
(contiguous all-NaN values) larger than `max_gap_size`. | ||
|
||
This method is only applicable to deterministic series (i.e., having 1 sample). | ||
Parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep the empty lines above "Parameters"
Works only on deterministic time series. | ||
|
||
A function to compute and return gaps in the TimeSeries. Works only on deterministic time series (1 sample). | ||
Parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
darts/timeseries.py
Outdated
Returns | ||
------- | ||
-------/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the /
for rendering.
Codecov ReportBase: 93.86% // Head: 93.84% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #1265 +/- ##
==========================================
- Coverage 93.86% 93.84% -0.03%
==========================================
Files 78 78
Lines 8495 8492 -3
==========================================
- Hits 7974 7969 -5
- Misses 521 523 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks!
Fixes #629, #647 and #1225
Summary
Took code suggested by @phubermi in #629, corrected it and added the corresponding tests.