Refactor forecasting models

[JanFidor]

Fixes #1586

Summary

For now I only pushed changes to baseline models

@hrzn Potential changes I wanted to get your feedback on before pushing:

1. Refactor all __str__ for all models to be their class name with meaningful params captured in parenthesis
   ex.ample: "Auto-ARIMA-Statsforecasts" -> "StatsForecastAutoARIMA"
2. Add logger to every raise_if and raise_if_not call which don't yet have it
3. Use fstring formatting wherever possible

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.06%. Comparing base (13b84c8) to head (15063a7).
Report is 292 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1590      +/-   ##
==========================================
- Coverage   94.13%   94.06%   -0.08%     
==========================================
  Files         125      125              
  Lines       11358    11300      -58     
==========================================
- Hits        10692    10629      -63     
- Misses        666      671       +5     

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

[dennisbader]

Thanks for this!

Actually, it could be super nice if we improved the representation of our models in general (in the __repr__ method). Maybe to something similar to how sklearn does it?

They represent models like this:

• default model without changing hparams:

  from sklearn.linear_models import LinearRegression
  model = LinearRegression()
  print(model)
  
  >>> LinearRegression()
  

• model with changed hyperparameters:

  from sklearn.linear_models import LinearRegression
  model = LinearRegression(fit_intercept=False)
  print(model)
  
  >>> LinearRegression(fit_intercept=False)
  

We already save the model parameters that the user manually sets at model creation. Accessible after model creation with:

model.model_params

What do you think? Would you be willing to give this a go?

[JanFidor]

Sure thing @dennisbader, btw while working on the other PR I noticed this line which imho looks like a raise_if() in disguise, should I convert it (and other similar asserts if I find them) to raise_if() / raise_if_not()?

[JanFidor]

@dennisbader what do you think about this implementation? If it's okay, I'll start deleting str overrides in children models

[dennisbader]

Sure thing @dennisbader, btw while working on the other PR I noticed this line which imho looks like a raise_if() in disguise, should I convert it (and other similar asserts if I find them) to raise_if() / raise_if_not()?

This is okay, raise_log is sometimes better to use, especially in for loops.
When we use formatted strings (f"something {val}") in for loops with raise_if or raisa_if_not, it can slow things down as the string needs to be computed every time.
With raise_log, the string is only evaluated if the condition is met.

[dennisbader]

Very neat, and looks great! Thanks

Had only a minor comment about renaming the method and moving it to the correct place.

You can go ahead and remove the model specific str methods!

[dennisbader]

👍 Very neat, thanks!

Could you rename the method into __repr__ and move it to the ForecastingModel base class?

[hrzn]

+1 for __repr__, however, it could also be beneficial to have __str__ setup correctly as well I guess (the two are called in different circumstances). WDYT?

[JanFidor]

I never really used __repr__ but if I understood it's point correctly (pretty much just a more informative __str__), then my intuition would be to make __str__ provide only the changed params while __repr__ provides them all, including the defaults. This way users can both get simple model labels for graph legends when comparing hyper-parameters and get the entire set of model's parameters without having to delve into documentation to check their default values. Would that make sense?

[dennisbader]

From what I understand the intention behind the magic methods is:

"""
The __str__() method is intended to return information in a readable format for humans to easily understand it. It is perfect for logging or displaying object information. While the __repr__() method's goal is to deliver a string representation of the object that may be used to replicate it, it is also designed to do the opposite.
(source)
"""

So in our case, the __str__ implementation in this PR can actually be used to create a new model object (it's doing what the __repr__ is intended to be used for).

From another source:
"""
If you don’t define a __str__() method for a class, then the built-in object implementation calls the __repr__() method instead.
"""

So I guess we would only have to define __repr__ and not __str__. It is perfectly fine though like you did it, I was just curious myself what the best practices were for these two magic methods :)

[hrzn]

Looks good!

[hrzn]

+1 for __repr__, however, it could also be beneficial to have __str__ setup correctly as well I guess (the two are called in different circumstances). WDYT?

[dennisbader]

Looks great, thanks a lot for this PR and the patience @JanFidor! 🚀

[JanFidor]

Hi @dennisbader , again thanks for the review and help, I'm just happy to make a little impact when it comes to ML : )