return metric score along with untrained best models & params

[khanetor]

Fixes #776.

Summary

The gridsearch method also returned the best metric score, aka the minimum error, along with the best model and parameters.

[codecov-commenter]

Codecov Report

Merging #822 (b48a8e6) into master (7ca7801) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #822   +/-   ##
=======================================
  Coverage   91.33%   91.33%           
=======================================
  Files          69       69           
  Lines        6867     6867           
=======================================
  Hits         6272     6272           
  Misses        595      595           

Impacted Files	Coverage Δ
darts/models/forecasting/forecasting_model.py	96.66% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ca7801...b48a8e6. Read the comment docs.

[hrzn]

This is a breaking change, but I think we can do it. The other option would be to introduce a parameter in the function to optionally return the best metric, but that would inflate the signature. So I think we could merge this change. Could you maybe just add a short unit test that verifies the actual value being returned in test_backtesting.py ?

[khanetor]

This is a breaking change, but I think we can do it. The other option would be to introduce a parameter in the function to optionally return the best metric, but that would inflate the signature. So I think we could merge this change. Could you maybe just add a short unit test that verifies the actual value being returned in test_backtesting.py ?

There is a function compare_best_against_random in file "test_backtesting.py", which seems to perform some score comparison between best params and random params. I was thinking that instead of recalculating best_score_1 and best_score_2, I can use the returned score from the new gridsearch function. Would that be a good approach in this case? One concern that I have is that best_model_1 gridsearch uses stride, but the best_score_1 backtesting only uses a default stride, so the scores might be different. Do you think we should add the usage of stride in the subsequent backtesting?

Another approach is to leave this as-is, and create another test where I also manually backtest the best model, and compare the score with the one returned by gridsearch function.

[khanetor]

Out of curiosity, there are some tests that you mark as skipUnless TORCH_AVAILABLE. I don't recall models such as RandomForest from sklearn requires PyTorch.

[hrzn]

This is a breaking change, but I think we can do it. The other option would be to introduce a parameter in the function to optionally return the best metric, but that would inflate the signature. So I think we could merge this change. Could you maybe just add a short unit test that verifies the actual value being returned in test_backtesting.py ?

There is a function compare_best_against_random in file "test_backtesting.py", which seems to perform some score comparison between best params and random params. I was thinking that instead of recalculating best_score_1 and best_score_2, I can use the returned score from the new gridsearch function. Would that be a good approach in this case? One concern that I have is that best_model_1 gridsearch uses stride, but the best_score_1 backtesting only uses a default stride, so the scores might be different. Do you think we should add the usage of stride in the subsequent backtesting?

Another approach is to leave this as-is, and create another test where I also manually backtest the best model, and compare the score with the one returned by gridsearch function.

I think in case of doubt it is better to create a new unit test dedicated to test this new functionality. My only concern is that ideally it shouldn't be too long-running; so maybe take a small series (e.g. air passengers), a model very quick to fit (e.g. Theta), and not too large a parameter space.

[hrzn]

Out of curiosity, there are some tests that you mark as skipUnless TORCH_AVAILABLE. I don't recall models such as RandomForest from sklearn requires PyTorch.

RandomForest does not depend on PyTorch, but it used to (we used to rely on PyTorch Datasets to train them, but that is no longer the case). It might be that we forgot to remove a couple of skipUnless clauses from the unit tests. If you find some and you are confident there's no PyTorch-based models involved, you can remove them in this PR, and I'll try testing (it will be tested by our CI pipelines upon merging anyway).

[khanetor]

Hello @hrzn . Anything else I should add? 😄

[hrzn]

Thanks!

[hrzn]

No it's good now, thanks for adding the test! Will merge once the tests are done, and it will be released when we release the new version of Darts. Thanks!