Fix/historical_forecasts callable retrain argument

[madtoinou]

Fixes #1655.

Summary

The retrain argument of historical_forecast() was not properly handled when it was a Callable, in order to support it appropriately the following changes has been made:

• the time index that can be used in "training" and "prediction" mode are computed separately, and if retrain is a Callable, the longest is used to build the iterator
• the retrain function is systematically called before initiating the training round, using the "dataset that would be used for training"
• the retrain function has a fixed signature with the following positional arguments: [counter, pred_time, train_series, past_covariates, future_covariates]

And the explanation behind the fact that any value <= 3 would mask the problem is linked to the fact that model.min_train_series_length takes the maximum between this constant and the output_chunk_length. Since this value could be longer than the model.min_predict_series_length, the Callable retrain was slicing the predictable time index too aggressively.

Other Information

I added some tests to cover the models with:

• "lags + future covariates, output_chunk_length = 1"
• "lags + future covariates, output_chunk_length > 3"
• "future covariates, output_chunk_length > 3"

But I am not entirely sure of the expected forecast's length for the last 2 scenarios, I need to run some additional tests.

Update 10-04-2023 (@dennisbader):

• move around start and train_length logic to account for all inputs
• improved error messages for start param sanity checks
• raises warning when valid start param but not in predictable/trainable index
• improvements to the way we handle train length, raises warning when too large
• option to disable warnings
• sets min samples required for torch models to 1 (only regression models require minimum of 2 samples)
• update docs for historical_forecasting, backtest, gridsearch, residuals

[codecov-commenter]

Codecov Report

Patch coverage: 90.74% and project coverage change: -0.19 ⚠️

Comparison is base (e4908f7) 94.18% compared to head (e3e90fa) 93.99%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1675      +/-   ##
==========================================
- Coverage   94.18%   93.99%   -0.19%     
==========================================
  Files         125      125              
  Lines       11393    11432      +39     
==========================================
+ Hits        10730    10746      +16     
- Misses        663      686      +23     

Impacted Files	Coverage Δ
darts/utils/__init__.py	100.00% <ø> (ø)
darts/utils/timeseries_generation.py	96.15% <ø> (ø)
darts/models/forecasting/ensemble_model.py	90.56% <66.66%> (-1.44%)	⬇️
darts/models/forecasting/forecasting_model.py	94.53% <88.00%> (-1.82%)	⬇️
darts/models/forecasting/dlinear.py	100.00% <100.00%> (ø)
darts/models/forecasting/regression_model.py	97.15% <100.00%> (+0.02%)	⬆️
darts/utils/utils.py	91.24% <100.00%> (+0.72%)	⬆️

... and 10 files with indirect coverage changes

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[dennisbader]

Hi @madtoinou and thanks for getting the retrain callable to work properly 🚀 🥳

I left some minor suggestions, mainly about adapting the error messages a bit, trying to avoid creating new time series as much as we can, and fixes to some potential issues.

Btw, @dumjax and I were working on the last historical_forecasts bug fixes, that should will come in parallel with this PR. Then we're finally ready to put everything together for the release, and concentrate on optimizing the method for the next release :)

Thanks for all the great work so far!

[madtoinou]

After a lot of experimentation with the timestamps of the output of historical_forecasts, I identified some bugs:

• for models using positive lags_future_covariates only: the first predictable timestamp is the first value of the input series, requires a dummy support ts for the predict method.
• the timestamp shift (min_timestamp_predict) should be affected by the lags in the past only
• the unittest with the "manual" boundaries were not allowing for enough granularity: the output_chunk_length is relevant only when retrain=True and cannot be used interchangeably with the largest positivelags_future_covariate
• the sanity check for historical_forecasts were not supporting models using only positive lags_future_covariates when retrain=False

[dennisbader]

Thanks @madtoinou , that looks really good :)

I think we're in the final round of review and can merge soon! 🚀

Just had a couple of minor suggesetions.

[dennisbader]

A comment about lines 975 - 983 (the ValueError "retrain is False...")

With the current implementation, it can be that there will be a prediction before training, even if the retrain_callable always returns True.

So for this message to work, we would need a global counter _counter, and train specific counter _counter_train. The train counter will count each time pred_time is in historical_forecasts_time_index_train. So it is only 0 when we are in the prediction phase before the first possible train index. Then we can check something like below (I'm not sure it covers all the cases yet)

• catch case when first iteration is prediction and it's before the first possible train index. -> suggest different start date (the first possible date that will work), or retrain=True | int, fit model before.
• catch case when retrain_func actually returns false in the first possible train iteration -> suggest a different retrain value, changing the function so it returns True in first iteration, train model before

# use retrained model if `retrain` is not training every step
model = model if model is not None else self

# model must be fit before the first prediction
if not _counter and not model._fit_called:
    raise_log(
        ValueError(
            f"model has not been fit before in first predict iteration at prediction point (in time) `{pred_time}` "
            f"Either call `fit()` before `historical_forecasts()`, set `retrain=True`, or use a different "
            f"`start` value. The first possible start value is: {min_timestep_train????}"
        ),
        logger,
    )
if not _counter_train and not model._fit_called:
    raise_log(
        ValueError(
            f"`retrain` is `False` in first train iteration at prediction point (in time) `{pred_time}` "
            f"and the model has not been fit before. Either call `fit()` before "
            f"`historical_forecasts()`, or use a different `retrain` value / modify the function "
            f"to return `True` in first iteration."
        ),
        logger,
    )

So we would have to remove the parts about

[madtoinou]

Reformulating the two cases for eventual other reviewers:

• the model is not trained for the first "predictable" timestamp -> user should either call fit() before historical_forecasts, modify the retrain argument so that it returns True at least once before/at this timestamp or change the start value to the suggested timestamp so that the model training can be triggered before the prediction round.
• retrain is False for the first "trainable" timestamp and the model was not trained before historical_forecasts -> user must modify the retrain argument.

[dennisbader]

Looks good 🚀 💯 Great job @madtoinou !