Refactor/backtesting #125

guillaumeraille · 2020-07-06T11:36:21Z

Fixes #DARTS-133.

Summary

Contain a proposal to refactor backtesting.

Add reviews and comments directly to PROPOSAL.md

Other Information

hrzn · 2020-07-06T20:04:31Z

That looks very good @guillaumeraille
+1 for having model.backtest() methods and +1 for the submodule darts.model_selection
Some very minor comments that I think of now: I think we could find better names for fcast_horizon_n and explore_models().

TheMP · 2020-07-07T06:52:58Z

PROPOSAL.md

+
+```python
+model = ExponentialSmoothing()
+historical_forecast = model.backtest(series, pd.Timestamp('19550101'), fcast_horizon_n=3, verbose=True))


What would be returned here? A pandas series or a new "Backtesting" object with some properties?

A time series containing the forecast values for series, when successively applying the specified model with the specified forecast horizon

But my favorite is the next use case example where it also directly return the residuals so that we can remove the forecast_residual method

Droxef · 2020-07-07T12:17:03Z

PROPOSAL.md

+We could also add the gridSearch logic directly on the model class as a `classmethod`:
+
+```python
+best_model = ExponentialSmoothing.gridsearch(series, fcast_horizon...)


I like this idea, as for some models, we must perform this search to find the best model (theta and ETS are good example, and I had to do one for 4Theta)
One other option to do the gridsearch would be to compare the fittedvalues when it exists, instead of the results of a backtest (faster and should assess the performance of the model as well)

in that case we could create a dedicated method for that on the model as well (cant be a classmethod if it needs some fitted value) what do you think ?

Yes it seems more appropriate. Moreover that the fittedvalues call is not necessarily consistent between models

I guess that's a possible solution to https://github.com/unit8co/darts/pull/123/files#r453315099

pennfranc · 2020-07-08T10:16:18Z

PROPOSAL.md

+forcasting anyway:
+
+```python
+historical_forecast, residuals = model.backtest(series, pd.Timestamp('19550101'), fcast_horizon_n=3, verbose=True))


I like it, definitely more elegant. My only concern is whether this is an intuitive way to get to the residuals from the perspective of a new user.

an other possibility would be to store residuals on the model and retrieve it on request but I believe residuals shouldn't belong to the model

pennfranc · 2020-07-08T10:19:58Z

PROPOSAL.md

+The main idea behind this proposal is to refactor `backtesting.py` by implementing it directly in the model 
+class. Here would be a quick list of changes proposed:
+
+- `backtest_forecasting`, `backtest_regression`, `backtest_gridsearch` -> moved in corresping model methods


Do you think backtest_gridsearch could be made to a static method of ForecastingModel? (Since it takes a class, and not an instance as argument)

How about having an abstract method ForecastingModel.hyperparams() that returns the params dict that backtest_gridsearch() would iterate over? This way model implementers would just have to implement this one method for the model to be grid-searchable. @guillaumeraille @pennfranc wdyt?

Not a bad idea but I think the params dict is more dataset / application dependent hence unkown at model implementation time dont you think ?

Right that's true. What I had in mind was more for this method to return decent default hyper-params, but we would still need to accept user-defined parameters.

TheMP · 2020-07-13T15:41:31Z

PROPOSAL.md

+---
+## Progress
+
+- [x] backtest_forcasting moved in model methods


hrzn · 2020-07-13T18:02:49Z

darts/models/forecasting_model.py

+
+    def _backtest_build_fit_and_predict_kwargs(self,
+                                               target_indices: Optional[List[int]],
+                                               component_index: Optional[int],


this doesn't seem to be used

You mean because it is overriden? It is used as default and not rewritten in the multivariate case so used

As we discussed, I was talking about component_index that is apparently not used in this method.

…anity check method

…xed documentation

darts/models/forecasting_model.py

darts/models/regression_model.py

hrzn · 2020-09-09T19:35:47Z

darts/models/theta.py

@@ -337,7 +337,7 @@ def select_best_model(ts: TimeSeries, thetas: Optional[List[int]] = None,
        using the fitted values on the training series `ts`.


-        Uses 'backtesting.backtest_gridsearch' with 'use_fitted_values=True' and 'metric=metrics.mae`.
+        Uses 'ForecastingModel.gridsearch' with 'use_fitted_values=True' and 'metric=metrics.mae`.


This function select_best_model is basically doing a gridsearch, but it's misplaced IMO, and I think we should remove it if we can.
Instead one option could be to have each model implement a method default_hyperparams returning some good ranges of hyper parameters to be iterated on when grid-searching. This way afterwards in ForecastingModel we could have only one method auto_select_model doing the gridsearch on reasonable hyper-params.

I agree, some standardization may be good here. But should we leave this for a separate PR?

darts/timeseries.py

…ocstrings, added missing checks

hrzn

Nice improvement :)

… corresponding test cases

…rameter in 'ForecastingModel.gridsearch'

* add support for columns to the TimeSeries object * add colum support indexing to timeseries * fix wrong docstring * refactor indexing, fix docstring, columns as last arg * clean indexing method * refactor indexing only based on loc and iloc * Update darts/timeseries.py Co-authored-by: Julien Herzen <julien.herzen@unit8.co> * use underlying columns by default * fix column added on intern _df and use self.freq_str * fix parameter position in from_times_and_values * fix the tests to use str columns * fix docstring timeseries * remove None check on df that should exists * add comment for clarifying that _df is a copy * add separate function to process columns * adapt map with str col indexing * univariate fcast model only support univariate ts * MultivariateFcasModel fits on the whole training ts * refactor torch forcasting model to use covariate_series * fix unused imports * allow to specify only covaraite_series * enforce covariate_series and target_series inputs for multivariate model * adapt torch datasets to use covariate / target series * adapt validation series provided as a Tuple * fix typo * adapt create_dataset on tcn model * remove component index from fit function * adapt tests to new syntax * refacotr metaclasses * abstract a new method make fitable series * adapt torchforcastingmodel to parent class changes * keep covariate/target seires for Multivariate models only * fix typos with new implementation * move series length check in forcasting model * rename covariate into training series * adapt old backtesting to support the new fit args syntax * Refactor/backtesting (#125) * add .DS_Store to .gitignore * add proposal.md * add draft version of backtest forcasting * add backtest to model (simple refactoring) * extract backtest sanity checks in a method * extract building fit_kwargs and predict_kwargs in a method * minor fix import comment and assertion * refactor all backtest factoring tests * update progress on proposal.md * add coverage.sh * fix permission on coverage.sh * improve coverage sh script * add coverage.xml to .igtignore * improve doc on coverage.sh * fix doc * fix doc for real * univariate fcast model only support univariate ts * MultivariateFcasModel fits on the whole training ts * refactor torch forcasting model to use covariate_series * fix unused imports * allow to specify only covaraite_series * enforce covariate_series and target_series inputs for multivariate model * adapt torch datasets to use covariate / target series * adapt validation series provided as a Tuple * fix typo * adapt create_dataset on tcn model * remove component index from fit function * adapt tests to new syntax * add proposal.md * add draft version of backtest forcasting * add backtest to model (simple refactoring) * extract backtest sanity checks in a method * extract building fit_kwargs and predict_kwargs in a method * minor fix import comment and assertion * refactor all backtest factoring tests * update progress on proposal.md * fix doc * fix doc for real * fix typos and remove diagram in backtest doc * WIP add residuals * add decorator for sanity checks * clean forecasting_model * add start multitype parameter support * fix check on undefined param in sanity checks * add comments * fix(backtesting, tests): fixed bugs so that all forecasting backtest tests pass, corrected some typos * feature(backtesting): changed handling of residuals (re-introduced own function instead of being by-product of backtest) * fix(test_forecasting_model): deleted old file that was renamed due to type * feat(backtesting): moved gridsearch to ForecastingModel, removed functions from backtesting module that have been moved to ForecastingModel class, adapted tests * feat(backtesting): adapted docstring of gridsearch function * fix(Theta): adapted FourTheta model to use new gridsearch function * fix(forecasting_model, torch_forecasting_model): fixed docstrings * feat(backtesting): moved backtest_regression to regression model class * fix(forecasting_model): renamed covariate_series to training_series * fix(forecasting_model): fixed residuals function * fix(style): linter * feat(backtesting): renamed backtest_gridsearch to gridsearch * fix(tests): fixed residuals test case * feat(backtesting): moved residuals plotting function to statistics module * feat(backtesting): removed backtesting module * fix(style): linter * fix(style): linter * fix(torch_forecasting_model): fixed check in predict function * fix(forecasting_model): fixed backtest sanity check * fix(torch_forecasting_model): removed unnecessary (and bug-causing) sanity check method * feat(examples): refactored notebooks to support new function signatures * fix(style): linter * updated PROPOSAL.md * feat(forecasting_model): improved documentation * fix(torch_forecasting_model): removed redundant function * style(torch_forecasting_model): linter * fix(torch_forecasting_model): fixed docstring typo * fix(torch_forecasting_model, tests): clean up old comments * fix(statistics): improved docstrings * fix(forecasting_model, regression_model): improved variable names, fixed documentation * fix(tests): fixed old variable name in backtesting tests * removed PROPOSAL.md * feat(regression_model): added stride functionality to backtest method * fix(forecasting_model, regression_model): improved documentation * fix(forecasting_model): improved documentation * fix(forecasting_model): improved start parameter documentation * fix(forecasting_model, regression_model): cleaned up code, improved docstrings, added missing checks * feat(forecasting_model): improved backtest docstring * fix(forecasting_model, tests): improved backtest sanity checks, added corresponding test cases * feat(backtesting): replaced 'num_predictions' parameter by 'start' parameter in 'ForecastingModel.gridsearch' * fix(examples): updated notebooks Co-authored-by: Guillaume Raille <guillaume.raille@unit8.co> Co-authored-by: pennfranc <flaessig@student.ethz.ch> Co-authored-by: Julien Herzen <julien.herzen@unit8.co> Co-authored-by: TheMP <marek.pasieka@gmail.com> Co-authored-by: Francesco Lässig <42946363+pennfranc@users.noreply.github.com> Co-authored-by: Guillaume <66320848+guillaumeraille@users.noreply.github.com> Co-authored-by: pennfranc <flaessig@student.ethz.ch>

grll added 2 commits July 6, 2020 08:53

add .DS_Store to .gitignore

7cb76d7

add proposal.md

36c70be

guillaumeraille requested review from hrzn, pennfranc, TheMP and Droxef July 6, 2020 11:37

TheMP reviewed Jul 7, 2020

View reviewed changes

Droxef reviewed Jul 7, 2020

View reviewed changes

pennfranc reviewed Jul 8, 2020

View reviewed changes

grll added 17 commits July 10, 2020 14:59

Merge branch 'develop' into refactor/backtesting

35dc823

add draft version of backtest forcasting

52ecf65

add backtest to model (simple refactoring)

1c51c16

extract backtest sanity checks in a method

c2ab0d5

extract building fit_kwargs and predict_kwargs in a method

99781e9

minor fix import comment and assertion

cb602c9

refactor all backtest factoring tests

b3dbbaa

update progress on proposal.md

8cd006c

add coverage.sh

b8d8b9b

fix permission on coverage.sh

1116182

improve coverage sh script

4f4888f

add coverage.xml to .igtignore

1f4e0de

improve doc on coverage.sh

7d5c9b3

Merge branch 'feature/coverage' into refactor/backtesting

55c6726

Merge branch 'develop' into refactor/backtesting

a996788

fix doc

8b92817

fix doc for real

b565350

TheMP reviewed Jul 13, 2020

View reviewed changes

PROPOSAL.md Outdated

---

## Progress

- [x] backtest_forcasting moved in model methods

Copy link

Contributor

TheMP Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

hrzn reviewed Jul 13, 2020

View reviewed changes

pennfranc added 15 commits September 1, 2020 15:59

fix(torch_forecasting_model): fixed check in predict function

4dbca4d

fix(forecasting_model): fixed backtest sanity check

4dcf2e1

fix(torch_forecasting_model): removed unnecessary (and bug-causing) s…

af8c3e8

…anity check method

feat(examples): refactored notebooks to support new function signatures

194a292

fix(style): linter

aa6119e

updated PROPOSAL.md

f98f2db

feat(forecasting_model): improved documentation

ed427ce

fix(torch_forecasting_model): removed redundant function

982d7dd

style(torch_forecasting_model): linter

4dc3eb4

fix(torch_forecasting_model): fixed docstring typo

d0fe97d

fix(torch_forecasting_model, tests): clean up old comments

70986a3

fix(statistics): improved docstrings

dbd9c92

fix(forecasting_model, regression_model): improved variable names, fi…

7a6b94c

…xed documentation

fix(tests): fixed old variable name in backtesting tests

bcb4af8

removed PROPOSAL.md

f51f4a2

pennfranc marked this pull request as ready for review September 8, 2020 08:22

hrzn reviewed Sep 9, 2020

View reviewed changes

pennfranc added 6 commits September 10, 2020 10:49

feat(regression_model): added stride functionality to backtest method

3a8df6a

fix(forecasting_model, regression_model): improved documentation

a75fca2

fix(forecasting_model): improved documentation

fb27490

fix(forecasting_model): improved start parameter documentation

d9b8633

fix(forecasting_model, regression_model): cleaned up code, improved d…

30fedc1

…ocstrings, added missing checks

feat(forecasting_model): improved backtest docstring

bf28eca

hrzn approved these changes Sep 15, 2020

View reviewed changes

pennfranc added 3 commits September 15, 2020 19:23

fix(forecasting_model, tests): improved backtest sanity checks, added…

4ed2ab6

… corresponding test cases

feat(backtesting): replaced 'num_predictions' parameter by 'start' pa…

3f63507

…rameter in 'ForecastingModel.gridsearch'

fix(examples): updated notebooks

8dac68e

pennfranc merged commit c75c9c4 into refactor/fit-args Sep 17, 2020

LeoTafti mentioned this pull request Sep 18, 2020

Fix/backtest sanity checks #187

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor/backtesting #125

Refactor/backtesting #125

guillaumeraille commented Jul 6, 2020

hrzn commented Jul 6, 2020

TheMP Jul 7, 2020

guillaumeraille Jul 7, 2020

guillaumeraille Jul 7, 2020

Droxef Jul 7, 2020

guillaumeraille Jul 7, 2020

Droxef Jul 7, 2020

hrzn Jul 12, 2020

pennfranc Jul 8, 2020

guillaumeraille Jul 8, 2020

pennfranc Jul 8, 2020

hrzn Jul 12, 2020

guillaumeraille Jul 13, 2020

hrzn Jul 13, 2020

TheMP Jul 13, 2020

hrzn Jul 13, 2020

guillaumeraille Jul 14, 2020

hrzn Jul 17, 2020

hrzn Sep 9, 2020

pennfranc Sep 10, 2020

hrzn left a comment

Refactor/backtesting #125

Refactor/backtesting #125

Conversation

guillaumeraille commented Jul 6, 2020

Summary

Other Information

hrzn commented Jul 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrzn left a comment

Choose a reason for hiding this comment