fix pass verbose to fit in historical_forecast

[turbotimon]

Checklist before merging this PR:

• Mentioned all issues that this PR fixes or addresses.
• Summarized the updates of this PR under Summary.
• Added an entry under Unreleased in the Changelog.

Fixes #2804.

Summary

verbose now get passed from historical_forecast onto fit (and not only predict) so progressbar etc are not shown during retrain

Other Information

[dennisbader]

Thanks for giving this a go @turbotimon 🚀 . This fails because not all models support the verbose parameter in fit().
We would have to change the API of ForecastingModel.fit() and subclasses to accept the verbose param even if it is not used.

On another thought, I think it could be valuable to make the verbose param of historical_forecasts() customizable to which level it should act on. For example, I assume that many users are interested in the global progress of the historical forecasts but not in the progress bars of fit and predict.

Basically verbose could accept either bool (show everything or nothing) and another type that specifies the level. Maybe an int in {-1, 0, 1} with -1 meaning show nothing, 0 means show only historical forecasts progress, and 1 means show everything). Just an idea. Let me know what you think :)

[turbotimon]

Oh, I checked a range of models of each domain like ARIMA, LinReg, some Torch and have seen that the most general type ForecastingModel supports verbose in fit. But I admit I should have run the test locally.

Regarding a solution:

1. "accept the verbose param even if it is not used": This would be the easiest solution and consistent. E.g. the Theta model already accept verbose in predict and does not use it. I don't think this hurts and would make the interface more general.

2. "specifies the level": I don't like this very much for the following reasons: 1. Needs explanation for the user 2. Needs more logic to implement and historical_forecast is already ~600 lines long. 2. Yet it gives no fine control (e.g. only fit but not predict).

3. But another option could be to allow passing verbose explicit in fit_kwargs and verbose_kwargs and it would override the verbosity received from historical_forecast for these methods. This would basically solve 2. but with more flexibility (e.g. only fit but not predict). But this solution would presume 1.

[dennisbader]

Alright, then go ahead with solution 1 :)

[turbotimon]

@dennisbader I added solution 1 with the following comments:

• verbose is currently not passed to super().fit in most cases, because it also not done in most predict
• verbose is only passed to the underlying self.model.fit() if the underlying model supports it
• a hack is needed for CatBoostModel, because it derived from SKLearnModel but SKLearnModel normally do not support verbose in their fit method (see code changes for details)
• I added verbose=False to a couple of test to proof if it can be passed without errors. I opted for not implementing additional test just for that to not increase test time.

What do you think?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.15%. Comparing base (44d1f7e) to head (97ca5d4).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2805      +/-   ##
==========================================
- Coverage   95.21%   95.15%   -0.06%     
==========================================
  Files         145      145              
  Lines       15057    15098      +41     
==========================================
+ Hits        14336    14366      +30     
- Misses        721      732      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dennisbader]

Thanks for the updates @turbotimon, it looks great now 🚀

Just had a couple minor suggestions, after that it's ready to be merged :)

[turbotimon]

@dennisbader sorry for the silence, I was heavily occupied, but i finally had time to implemented your suggestions

[dennisbader]

Thanks for the great updates @turbotimon and also sorry for the waiting - I was on holidays :) Everything looks good.

I quickly checked the new behavior for a TorchForecastingModel and now I have to admit that your solution 3. from a while ago seems like the preferred option.

3. But another option could be to allow passing verbose explicit in fit_kwargs and predict_kwargs and it would override the verbosity received from historical_forecast for these methods. This would basically solve 2. but with more flexibility (e.g. only fit but not predict). But this solution would presume 1.

I also think that it should be possible to deactivate the fit progress bar while still showing the historical forecast progress (as you mentioned in the original issue).

Would it be okay for you to apply this suggestion? I could also do the changes if you prefer that :)

Sorry for the back and forth. I hope it doesn't inflict too much extra effort 🙏

[turbotimon]

@dennisbader I'm also in favor of solution 3 to be honest :) But unfortunately, I probably won't have time for that at the moment, so please go one.

I would continue to propose that ‘verbose’ remain included in all fit/predict methods for all models (even if it has no effect for some of them). This would make the interfaces more consistent across models and could be beneficial in the future. (If not, at least consistent for model families like e.g. GlobalForecastingModel)

[dennisbader]

@turbotimon perfext, and I agree with keeping the params in the API.

Sounds good, I'll take it over. Thanks again for the great work!