Repository: sktime/sktime
Pull request: [ENH] Add `fit_kwargs` to Prophet
<!--
Welcome to sktime, and thanks for contributing!
Please have a look at our contribution guide:
https://www.sktime.net/en/latest/get_involved/contributing.html
-->

#### Reference Issues/PRs
<!--
Example: Fixes #1234. See also #3456.

Please use keywords (e.g., Fixes) to create link to the issues or pull requests
you resolved, so that they will automatically be closed when your pull request
is merged. See https://github.com/blog/1506-closing-issues-via-pull-requests.
If no issue exists, you can open one here: https://github.com/sktime/sktime/issues
-->

Fixes #2344 

#### What does this implement/fix? Explain your changes.
<!--
A clear and concise description of what you have implemented.
-->

Adds a dict `fit_kwargs` parameter to the Prophet forecaster constructor. This dictionary is then passed down to the `prophet.forecaster.Prophet.fit` method.

#### Does your contribution introduce a new dependency? If yes, which one?

<!--
Only relevant if you changed pyproject.toml.
We try to minimize dependencies in the core dependency set. There
are also further specific instructions to follow for soft dependencies.
See here for handling dependencies in sktime: https://www.sktime.net/en/latest/developer_guide/dependencies.html
-->

No

#### Did you add any tests for the change?

<!-- This section is useful if you have added a test in addition to the existing ones. This will ensure that further changes to these files won't introduce the same kind of bug. It is considered good practice to add tests with newly added code to enforce the fact that the code actually works. This will reduce the chance of introducing logical bugs.
-->

Added a test that checks whether the correct kwargs are passed down to the `prophet.forecaster.Prophet.fit` method.

#### Any other comments?
<!--
We value all user contributions, no matter how small or complex they are. If you have any questions, feel free to post
in the dev-chat channel on the sktime discord https://discord.com/invite/54ACzaFsn7. If we are slow to review (>3 working days), likewise feel free to ping us on discord. Thank you for your understanding during the review process.
-->

No

#### PR checklist
<!--
Please go through the checklist below. Please feel free to remove points if they are not applicable.
-->

##### For all contributions
- [x] I've added myself to the [list of contributors](https://github.com/sktime/sktime/blob/main/CONTRIBUTORS.md) with any new badges I've earned :-)
  How to: add yourself to the [all-contributors file](https://github.com/sktime/sktime/blob/main/.all-contributorsrc) in the `sktime` root directory (not the `CONTRIBUTORS.md`). Common badges: `code` - fixing a bug, or adding code logic. `doc` - writing or improving documentation or docstrings. `bug` - reporting or diagnosing a bug (get this plus `code` if you also fixed the bug in the PR).`maintenance` - CI, test framework, release.
  See here for [full badge reference](https://allcontributors.org/docs/en/emoji-key)
- [x] Optionally, I've added myself and possibly others to the [CODEOWNERS](https://github.com/sktime/sktime/blob/main/CODEOWNERS) file - do this if you want to become the owner or maintainer of an estimator you added.
  See here for further details on the [algorithm maintainer role](https://www.sktime.net/en/latest/get_involved/governance.html#algorithm-maintainers).
- [x] The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.

##### For new estimators
- [ ] I've added the estimator to the API reference - in `docs/source/api_reference/taskname.rst`, follow the pattern.
- [ ] I've added one or more illustrative usage examples to the docstring, in a pydocstyle compliant `Examples` section.
- [ ] If the estimator relies on a soft dependency, I've set the `python_dependencies` tag and ensured
  dependency isolation, see the [estimator dependencies guide](https://www.sktime.net/en/latest/developer_guide/dependencies.html#adding-a-soft-dependency).

<!--
Thanks for contributing!
-->


Problem statement:
[ENH] Pass `pmdarima` and `prophet` models' `fit_params` to constructor
Native (not `sktime`) `pmdarima` models `ARIMA` and `AutoARIMA`, and `Prophet` (in`prophet`, not `sktime`) have some parameters which can be passed via `fit_params` (natively in `pmdarima` or `prophet`).
Users should be able to pass those parameters through the constructor of the `sktime` interface classes.
We should:

* check whether we are missing any parameters in the constructors that can be passed as `fit_params`. (if none are missing, we do not need to do the second step for that class) 
    - [x] `Prophet`
    - [ ] `ARIMA`
    - [ ] `AutoARIMA` 
* if yes, add them to the constructor and ensure they are passed on to the interfaced model. The recipe is as below.
    - [x] `Prophet`
    - [ ] `ARIMA`
    - [ ] `AutoARIMA` 
 
Good first issue since recipe is available.

---
Recipe for interfacing models with `fit_params` or any other `fit` args, in sktime:

* pass the parameter(s) to the constructor (the `__init__` method), using explicit parameter names and default, not `**kwargs`
* write it to `self` in the constructor
* in `_fit`, read it from `self` and pass it to the wrapped model's `fit` method
* ensure docstring is complete and defaults documented (optimally same as in wrapped estimator)

This allows to expose all parameter choices via the scikit-learn compatible interface (which relies on the constructor to expose all parameter choices).

For an example of this, you can look at `smoothing_level` in `ExponentialSmoothing`.

Hints from issue discussion:
Can I work on this issue @fkiraly ?
@Ris-Bali yes pls 🙂
FYI @jasminelaw, duplicate of your #3687.

Contributions appreciated!
@fkiraly I would like to pick this up for the Prophet model. How would you like the interface to look like? I was thinking that a `fit_kwargs` would work best here since these parameters can get passed down to different Stan backends which can accept different arguments with different default values. Here's an example implementation:

In `sktime/forecasting/fbprophet.py`:
```python
class Prophet(_ProphetAdapter):
    def __init__(
        self,
        # ...
        fit_kwargs=None,
    ):
    # ...
    self.fit_kwargs = fit_kwargs or {}
```

and then in `sktime/forecasting/base/adapters/_fbprophet.py::_ProphetAdapter._fit()`:
```python
        if self.verbose:
            self._forecaster.fit(df=df, **self.fit_kwargs)
        else:
            with _suppress_stdout_stderr():
                self._forecaster.fit(df=df, **self.fit_kwargs)
```


Let me know what you think :) 
@tpvasconcelos, looks reasonable - so you are saying we *are* missing parameters in `Prophet`?

a dict of params makes sense, although it tends to be unpleasant when using in tuners etc.
@fkiraly Yes that seems to be the case.

As you can see, Prophet's `fit` method accepts arbitrary (and undocumented) parameters that are passed down to Stan.

https://github.com/facebook/prophet/blob/c00f6a2d72229faa6acee8292bc01e14f16f599c/python/prophet/forecaster.py#L1188-L1212
```python
    def fit(self, df, **kwargs):
        """Fit the Prophet model.
        ...

        Parameters
        ----------
        df: pd.DataFrame containing the history. Must have columns ds (date
            type) and y, the time series. If self.growth is 'logistic', then
            df must also have a column cap that specifies the capacity at
            each ds.
        kwargs: Additional arguments passed to the optimizing or sampling
            functions in Stan.
        ...
        """
```

and sktime does not pass anything as of now:

https://github.com/sktime/sktime/blob/df21a0c0275ebf28deb30efac5d469c9f0d178e3/sktime/forecasting/base/adapters/_fbprophet.py#L119-L123

I agree with your point but unfortunately, since these are undocumented and even the Prophet API accepts an arbitrary dictionary, I think we should follow suit here.

From my side, I only need this for testing purposes as this is the only way to set a random seed and select the underlying optimiser used by stan (non are deterministic but `algorithm='Newton'` yields the most consistent results). AFAIK, these would not be used for hyperparameter tuning.

I'll get started on a PR 👍

Commit message: Add `fit_kwargs` to `get_test_params`

Patch to review:
diff --git a/sktime/forecasting/base/adapters/_fbprophet.py b/sktime/forecasting/base/adapters/_fbprophet.py
index acfd91f4707..79ac65bf044 100644
--- a/sktime/forecasting/base/adapters/_fbprophet.py
+++ b/sktime/forecasting/base/adapters/_fbprophet.py
@@ -116,11 +116,12 @@ def _fit(self, y, X, fh):
             df["cap"] = self.growth_cap
             df["floor"] = self.growth_floor
 
+        fit_kwargs = self.fit_kwargs or {}
         if self.verbose:
-            self._forecaster.fit(df=df)
+            self._forecaster.fit(df=df, **fit_kwargs)
         else:
             with _suppress_stdout_stderr():
-                self._forecaster.fit(df=df)
+                self._forecaster.fit(df=df, **fit_kwargs)
 
         return self
 
diff --git a/sktime/forecasting/fbprophet.py b/sktime/forecasting/fbprophet.py
index e814a8b164f..95b2809cee5 100644
--- a/sktime/forecasting/fbprophet.py
+++ b/sktime/forecasting/fbprophet.py
@@ -41,6 +41,8 @@ class Prophet(_ProphetAdapter):
         Dict with args for Prophet.add_country_holidays().
         Dict can have the following keys/values:
             country_name: Name of the country, like 'UnitedStates' or 'US'
+    fit_kwargs: dict or None, default=None
+        Dict with args for Prophet.fit().
     growth: str, default="linear"
         String 'linear' or 'logistic' to specify a linear or logistic
         trend. If 'logistic' specified float for 'growth_cap' must be provided.
@@ -138,6 +140,7 @@ def __init__(
         freq=None,
         add_seasonality=None,
         add_country_holidays=None,
+        fit_kwargs=None,
         # Args of fbprophet
         growth="linear",
         growth_floor=0.0,
@@ -162,6 +165,7 @@ def __init__(
         self.freq = freq
         self.add_seasonality = add_seasonality
         self.add_country_holidays = add_country_holidays
+        self.fit_kwargs = fit_kwargs
 
         self.growth = growth
         self.growth_floor = growth_floor
@@ -233,5 +237,6 @@ def get_test_params(cls, parameter_set="default"):
             "daily_seasonality": False,
             "uncertainty_samples": 10,
             "verbose": False,
+            "fit_kwargs": {"seed": 12345},
         }
         return params
diff --git a/sktime/forecasting/tests/test_prophet.py b/sktime/forecasting/tests/test_prophet.py
index e1cc1012791..677373d5ba3 100644
--- a/sktime/forecasting/tests/test_prophet.py
+++ b/sktime/forecasting/tests/test_prophet.py
@@ -3,7 +3,9 @@
 # copyright: sktime developers, BSD-3-Clause License (see LICENSE file)
 """
 
-__author__ = ["fkiraly"]
+__author__ = ["fkiraly", "tpvasconcelos"]
+
+from unittest import mock
 
 import pandas as pd
 import pytest
@@ -77,3 +79,29 @@ def test_prophet_period_fh(convert_to_datetime):
     else:
         assert isinstance(y_pred.index, pd.PeriodIndex)
         assert (y_pred.index == fh_index).all()
+
+
+@pytest.mark.skipif(
+    not run_test_for_class(Prophet),
+    reason="run test only if softdeps are present and incrementally (if requested)",
+)
+@pytest.mark.parametrize(
+    "fit_kwargs", [None, {"foo": "bar"}, {"foo1": "bar1", "foo2": "bar2"}]
+)
+def test_prophet_fit_kwargs_are_passed_down(fit_kwargs: dict):
+    """Test that the `fit_kwargs` hyperparameter is passed down to Prophet.fit()."""
+    from sktime.datasets import load_airline
+    from sktime.forecasting.fbprophet import Prophet
+
+    y = load_airline()
+    with mock.patch("prophet.forecaster.Prophet.fit") as mock_fit:
+        forecaster = Prophet(fit_kwargs=fit_kwargs)
+        forecaster.fit(y)
+        mock_fit.assert_called_once
+        mock_fit.assert_called_once()
+        assert mock_fit.call_args.args == ()
+        call_kwargs = mock_fit.call_args.kwargs
+        # `df` should always be one of the arguments but
+        # we don't care about its actual value here
+        call_kwargs.pop("df")
+        assert call_kwargs == (fit_kwargs or {})


Code review comments:
diff_hunk: @@ -138,6 +140,7 @@ def __init__(
         freq=None,
         add_seasonality=None,
         add_country_holidays=None,
+        fit_kwargs=None,
path: sktime/forecasting/fbprophet.py
text: @user1:
We should add it at the end, because adding the param between others is a breaking change that would need deprecation.
Why: suppose a user uses args, not kwargs, in the constructor.

@author:
Makes sense. Done ✅