Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Passing kwargs to the underlying fitting function #2438

Open
DavidKleindienst opened this issue Jul 2, 2024 · 1 comment · May be fixed by #2460
Open

Passing kwargs to the underlying fitting function #2438

DavidKleindienst opened this issue Jul 2, 2024 · 1 comment · May be fixed by #2460
Labels
good first issue Good for newcomers improvement New feature or improvement pr_welcome Open to be worked on

Comments

@DavidKleindienst
Copy link
Contributor

DavidKleindienst commented Jul 2, 2024

Is your feature request related to a current problem? Please describe.
I wanted to pass some additional arguments to Prophet's fit function (from now on called fit_kwargs), but this is not currently supported by darts. The only current possibility for the user to achieve this, is to subclass Prophet and overwrite the ._fit function

In general, there are multiple different strategies (depending on the model) in darts how passing through of fit_kwargs is currently handled.
Looking through the code, I would summarize the current situation as follows (hope I haven't missed something):

  • RegressionModels support passing of fit_kwargs to the .fit function
  • TorchForecastingModels use the pl_trainer_kwargs argument in the constructor
  • ExponentialSmoothing supports passing of fit_kwargs to the constructor. It also allows passing of constructor_kwargs (that will be passed to the constructor of the underlying model) as a dict.
  • Prophet and AutoARIMA do not support passing of fit_kwargs (even though the underlying model has meaningful kwargs)
  • Other models do not support passing fit_kwargs, but that's fine since the underlying models do not support any meaningful additional arguments

Describe proposed solution
I propose to unify the behavior (except for TorchForecastingModels which makes sense be treated differently) to support passing of fit_kwargs through the .fit function, i.e.:

  • Add passing through of fit_kwargs to Prophet and AutoARIMA
  • Rework ExponentialSmoothing constructor and fit function signatures to the same style. This would be a breaking change.

I think having the argument passing in the .fit function rather than the constructor function is better for two reasons:

  1. Models often also support kwargs that are passed to the underlying models constructor, making a distinction between constructor_kwargs and fit_kwargs necessary. That means at least one of them has do be passed as a dict, which feels unintuive.
  2. I think the .fit method would be the more obvious place where users would look for the possibility to pass such kwargs.

Describe potential alternatives

  • Add passing through of fit_kwargs to Prophet and AutoARIMA but keep ExponentialSmoothing as it is
  • Add a fit_kwargs argument to the constructors of Prophet and AutoARIMA and pass those to the fit function.

Additional context
I'm happy to prepare a PR for this issue, once it is decided which of these solutions should be implemented

@DavidKleindienst DavidKleindienst added the triage Issue waiting for triaging label Jul 2, 2024
@dennisbader
Copy link
Collaborator

Hi @DavidKleindienst, and thanks for this issue. It's true indeed that we should unify more in this matter.
I like your proposed solution with opening up the fit kwargs in fit() (and removing it from ExponentialSmoothing constructor). As you say, this concerns all models except TorchForecastingModel.

You can go ahead with the PR 🚀 :)

@madtoinou madtoinou added good first issue Good for newcomers improvement New feature or improvement pr_welcome Open to be worked on and removed triage Issue waiting for triaging labels Jul 5, 2024
@DavidKleindienst DavidKleindienst linked a pull request Jul 16, 2024 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers improvement New feature or improvement pr_welcome Open to be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants