Feat/historical_forecasts accept negative integer as start value #1866
Conversation
madtoinou
changed the title
madtoinou
changed the title
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## master #1866 +/- ##
==========================================
- Coverage 93.87% 93.82% -0.06%
==========================================
Files 132 132
Lines 12677 12692 +15
==========================================
+ Hits 11901 11908 +7
- Misses 776 784 +8
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
That's a nice attempt at supporting start < 0.
However, changing the method get_timestamp_at_point() leads to ambiguous results as it's supposed to return the index position where the index value corresponds to the point value. For integer indexed TimeSeries that start at a negative index value, the method would result in the wrong index position.
Can we avoid this somehow?
Also when I read the historical_forecasting documentation when start is an integer:
If an ``int``, the parameter will be treated as an integer index to the time index of
`series` that will be used as first prediction time.
As far as I can tell if we have a series with RangeIndex, then start must exist in the RangeIndex. So also there having an integer start can be ambiguous as it can either mean the position of the actual index value or just the index position.
…argument in historical_forecasts. Updated exception accordingly
…rting with a negative value
There was a problem hiding this comment.
Looks very nice, thanks a lot @madtoinou , only had really minor comments.
Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>
There was a problem hiding this comment.
Thanks @madtoinou this also looks great 🚀
Just some minor suggestions mainly about the documentation. After those we can merge
| @@ -560,6 +571,7 @@ def historical_forecasts( | |||
| num_samples: int = 1, | |||
| train_length: Optional[int] = None, | |||
| start: Optional[Union[pd.Timestamp, float, int]] = None, | |||
| start_format: Literal["positional_index", "value_index"] = "value_index", | |||
There was a problem hiding this comment.
I would prefer ["position", "value"] to make it shorter, WDYT?
Also, can we make this only effective when the series is indexed with is a range index?
For DatetimeIndex, start is well defined for all types. I assume that most of the use cases are using DatetimeIndex, and it would be easier if they didn't have to change the start_format when they want to pass an integer.
Only for range index we need to know if it's a position or value.
There was a problem hiding this comment.
Completely agree!
Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>
Summary
historical_forecasts()to receive a negative integer asstartvalue by extending theget_index_at_value()method.Other Information
This feature is to be used in
EnsembleModelso thatforecasting_modelscan callhistorical_forecasts()instead ofpredict(), which should improve the results depending on the models being ensembled.