Fix #1049 - add prior_scale and mode arguments to prophet model's add_seasonality #1829

id5h · 2023-06-14T06:59:46Z

Fixes #1049 .

Summary

When fitting a prophet model, the add_seasonality function will now properly add the prior_scale and mode to the custom seasonality, in accordance with the behaviour described in the docs.

Other Information

…l's add_seasonality

codecov-commenter · 2023-06-14T08:04:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.95%. Comparing base (a5560cc) to head (40e9c26).
Report is 227 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1829   +/-   ##
=======================================
  Coverage   93.95%   93.95%           
=======================================
  Files         125      125           
  Lines       11773    11782    +9     
=======================================
+ Hits        11061    11070    +9     
  Misses        712      712

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

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

madtoinou

Hi @id5h,

Thank you for contributing to darts. Your PR solves the issue of the "not passed" prior_scale and mode arguments, however it does not fully address the linked issue as condition_name remain unsupported. Would it be possible to also cover it in this PR?

id5h · 2023-06-14T18:20:37Z

Hi @madtoinou ,

I pushed a simple update, where one can add conditional seasonalities as well.
I am not using the condition_name as an argument in the add_seasonality functionality. That is because in the FB prophet API that argument is used to identify the column that holds the mask - and that is not really necessary when using a univariate TimeSeries object.

id5h · 2023-06-19T12:27:31Z

Hi @madtoinou ,

It seems like the failed check is due to an issue with codecov and not with the commited code?
Am I correct, or should I take any other action?

madtoinou · 2023-06-19T14:58:22Z

Hi @id5h,

Sorry, last week was quite intense. I hope to be able to find some time during the week to review the code and run some tests.

I just re-launched the tests, let's hope Codedev does not fail (it has nothing to do with your code).

id5h · 2023-06-19T17:16:46Z

Hi @id5h,

Sorry, last week was quite intense. I hope to be able to find some time during the week to review the code and run some tests.

I just re-launched the tests, let's hope Codedev does not fail (it has nothing to do with your code).

Alright. Thanks for the clarification and the prompt reply :)

dennisbader

Hi @id5h, thanks a lot for this and sorry for the waiting time 🚀
Looks good to me. I would only suggest to actually use condition_name similar to how Prophet does it.
At fit/predict time we can check that the user supplied the condition in the future_covariates series.
What do you think?

darts/models/forecasting/prophet_model.py

dennisbader · 2023-07-03T10:52:34Z

darts/models/forecasting/prophet_model.py

@@ -318,13 +334,20 @@ def add_seasonality(
        fourier_order: int,
        prior_scale: Optional[float] = None,
        mode: Optional[str] = None,
+        condition_func: Optional[types.FunctionType] = None,


I would suggest to use condition_name instead of a function and check at fit/predict time that the future_covariates series contains the boolean (binary) component component_name.

For this to work we also need to avoid adding the conditions as regressors in the following line:

darts/darts/models/forecasting/prophet_model.py

Line 200 in b63543b

self.model.add_regressor(covariate)

I did a quick test and it seems to work fine.

Moved in latest commit to use the condition_name and future_covariates instead of sending a function

dennisbader · 2023-07-03T10:55:11Z

Also, could you add an entry to the unreleased section in CHANGELOG.md? Thanks!
And don't worry about the failing unit tests, we're working on this in another PR.

id5h · 2023-07-04T12:19:29Z

Hi @dennisbader , thanks for the review. I implemented the changes you requested and added a test for the new functionality.

dennisbader

Very nice, thanks @id5h .

Just some minor comments and then we can merge 🚀

darts/models/forecasting/prophet_model.py

dennisbader · 2023-07-05T10:34:40Z

darts/models/forecasting/prophet_model.py

+                raise_if(
+                    future_covariates is None,
+                    f"Condition name '{attributes['condition_name']}' is required by "
+                    f"the custom seasonality '{seasonality_name}', but future_covariates is None. In addition, "
+                    f"the model should be re-trained with future_covariates.",
+                    logger,
+                )


can be dropped as it's enforced in the parent class that user must supply future covariates to predict after having fit the model with future covariates

Suggested change

raise_if(

future_covariates is None,

f"Condition name '{attributes['condition_name']}' is required by "

f"the custom seasonality '{seasonality_name}', but future_covariates is None. In addition, "

f"the model should be re-trained with future_covariates.",

logger,

)

This will be raised in case a user, for whatever reason, fits a model, calls add_seasonality() after fitting and then predict(). Hence the "model should be retrained with future_covariates" message.

The error is now raised according to the logic in _check_seasonality_conditions()

dennisbader · 2023-07-05T10:38:51Z

darts/models/forecasting/prophet_model.py

        for seasonality_name, attributes in self._add_seasonalities.items():
+            condition_name = attributes["condition_name"]


Can we add a private method _check_seasonality_conditions() that checks if all the conditions are in the future covariates and that the conditions are binary? The method can then return the condition columns for the downstream logic.
We use this in fit and predict so it helps to have the logic in a common method.

This should loop through all seasonalities first and capture all missing/invalid conditions before raising the error. It's easier for the user to see when he has multiple missing conditions

Makes sense 👍
Done.

dennisbader · 2023-07-05T11:04:00Z

darts/tests/models/forecasting/test_prophet.py

+
+        model.fit(ts, future_covariates=future_covariates)
+
+        forecast = model.predict(30, future_covariates=future_covariates)


also here, predicting 7 days should already be enough

dennisbader · 2023-07-05T11:06:23Z

darts/tests/models/forecasting/test_prophet.py

can you add tests that check that missing condition columns in future covariates and non-binary columns raise an error?

an example for this:

with pytest.raises(ValueError): model.fit(..., future_covariates=invalid_future_covariates) ...

dennisbader · 2023-07-05T11:11:28Z

CHANGELOG.md

@@ -29,6 +29,7 @@ but cannot always guarantee backwards compatibility. Changes that may **break co
 - Fixed a bug when loading the weights of a `TorchForecastingModel` trained with encoders or a Likelihood. [#1744](https://github.com/unit8co/darts/pull/1744) by [Antoine Madrona](https://github.com/madtoinou).
 - Fixed a bug when using selected `target_components` with `ShapExplainer. [#1803](https://github.com/unit8co/darts/pull/#1803) by [Dennis Bader](https://github.com/dennisbader).
 - Fixed `TimeSeries.__getitem__()` for series with a RangeIndex with start != 0 and freq != 1. [#1868](https://github.com/unit8co/darts/pull/#1868) by [Dennis Bader](https://github.com/dennisbader).
+- Fixed an issue with `prophet_model.Prophet.add_seasonality()` to allow proper use of all passed parameters. [#1829](https://github.com/unit8co/darts/pull/#1829) by [Idan Shilon](https://github.com/id5h).


I would add this to the model improvements section.
"Prophet now supports conditional seasonalities, and properly handles all parameters passed to Prophet.add_seasonality() and model creation parameter add_seasonalities. ..."

Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>

…ng fit() and predict()

…ions

dennisbader

Really nice, thanks @id5h 🚀
Last comment about slightly changing the error message and then we can merge 💯

darts/models/forecasting/prophet_model.py

id5h · 2023-07-07T10:53:08Z

Great, thanks @dennisbader .
I will change to raise_log today most likely.

I have one suggestion before merging.
Because prophet accepts non-integer seasonalities, it would be nice to allow it also in darts.
I think the required change is minimal - the seasonal_periods type hint in add_seasonality() (line 369) and the "dtype" value in the seasonality_properties dictionary (line 469) need to be changed to Union[int, float].

What do you think about adding this to the current PR?

Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>

id5h · 2023-07-07T19:58:04Z

The raise_log() is imported now.
I also pushed the changes to accept float seasonality periods, including a small modification to the test - I hope that's fine with you.

darts/models/forecasting/prophet_model.py

dennisbader

Thanks for the contribution @id5h 🚀 I applied the last suggestion. We're good to merge now 💯

darts/models/forecasting/prophet_model.py

Fix unit8co#1049 - add prior_scale and mode arguments to prophet mode…

c3ad7ce

…l's add_seasonality

id5h requested review from hrzn and dennisbader as code owners June 14, 2023 06:59

madtoinou reviewed Jun 14, 2023

View reviewed changes

Add option to treat seasonality as conditional

a6d53f0

id5h requested a review from madtoinou June 15, 2023 10:00

Merge branch 'master' into fix/prophet-add-seasonality

9d80f0b

dennisbader requested changes Jul 3, 2023

View reviewed changes

Idan-QL and others added 4 commits July 4, 2023 14:09

Add seasonality conditions with a condition_name and future_covariates

5713aba

Add test for custom conditional seasonality

cf55740

Add entry for pr unit8co#1829

3d5fc4a

Merge branch 'master' into fix/prophet-add-seasonality

0638f1d

Merge branch 'master' into fix/prophet-add-seasonality

c6f9ee6

dennisbader requested changes Jul 5, 2023

View reviewed changes

id5h and others added 6 commits July 5, 2023 14:43

Update darts/models/forecasting/prophet_model.py

801e80a

Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>

Update darts/models/forecasting/prophet_model.py

5b0321c

Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>

Validate seasonality considitions through a private method when calli…

50a70d4

…ng fit() and predict()

Reduce predict horizon to 7. Add tests for missing and invalid condit…

0fe11d2

…ions

Move entry to models improvements section

a96a15f

Merge branch 'master' into fix/prophet-add-seasonality

c4cdbf2

dennisbader requested changes Jul 7, 2023

View reviewed changes

darts/models/forecasting/prophet_model.py Outdated Show resolved Hide resolved

Update err msg in _check_seasonality_conditions

e9418a6

Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>

Idan-QL added 2 commits July 7, 2023 21:48

Import raise_log. Initialize formatted str when necessary.

9057345

Accept float seasonalities as well. Update test

157f206

Merge branch 'master' into fix/prophet-add-seasonality

729abe2

dennisbader requested changes Jul 10, 2023

View reviewed changes

darts/models/forecasting/prophet_model.py Outdated Show resolved Hide resolved

Idan-QL and others added 3 commits July 10, 2023 13:35

Fix dtype of seasonal_periods. Update docstrings.

7c03f75

Merge branch 'master' into fix/prophet-add-seasonality

6fbbd4b

update docstring

61abeea

dennisbader approved these changes Jul 17, 2023

View reviewed changes

darts/models/forecasting/prophet_model.py Outdated Show resolved Hide resolved

Merge branch 'master' into fix/prophet-add-seasonality

40e9c26

dennisbader merged commit 7f428dd into unit8co:master Jul 18, 2023

madtoinou mentioned this pull request Aug 16, 2024

Support float seasonal_periods in Prophet #1828

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1049 - add prior_scale and mode arguments to prophet model's add_seasonality #1829

Fix #1049 - add prior_scale and mode arguments to prophet model's add_seasonality #1829

id5h commented Jun 14, 2023

codecov-commenter commented Jun 14, 2023 •

edited by codecov bot

Loading

madtoinou left a comment

id5h commented Jun 14, 2023

id5h commented Jun 19, 2023 •

edited

Loading

madtoinou commented Jun 19, 2023

id5h commented Jun 19, 2023

dennisbader left a comment •

edited

Loading

dennisbader Jul 3, 2023

id5h Jul 4, 2023

dennisbader commented Jul 3, 2023 •

edited

Loading

id5h commented Jul 4, 2023

dennisbader left a comment

dennisbader Jul 5, 2023

id5h Jul 5, 2023

id5h Jul 5, 2023

dennisbader Jul 5, 2023

id5h Jul 5, 2023

dennisbader Jul 5, 2023

id5h Jul 5, 2023

dennisbader Jul 5, 2023

id5h Jul 5, 2023

dennisbader Jul 5, 2023

id5h Jul 5, 2023

dennisbader left a comment

id5h commented Jul 7, 2023

id5h commented Jul 7, 2023

dennisbader left a comment

		for seasonality_name, attributes in self._add_seasonalities.items():
		condition_name = attributes["condition_name"]


		model.fit(ts, future_covariates=future_covariates)

		forecast = model.predict(30, future_covariates=future_covariates)

Fix #1049 - add prior_scale and mode arguments to prophet model's add_seasonality #1829

Fix #1049 - add prior_scale and mode arguments to prophet model's add_seasonality #1829

Conversation

id5h commented Jun 14, 2023

Summary

Other Information

codecov-commenter commented Jun 14, 2023 • edited by codecov bot Loading

Codecov Report

madtoinou left a comment

Choose a reason for hiding this comment

id5h commented Jun 14, 2023

id5h commented Jun 19, 2023 • edited Loading

madtoinou commented Jun 19, 2023

id5h commented Jun 19, 2023

dennisbader left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dennisbader commented Jul 3, 2023 • edited Loading

id5h commented Jul 4, 2023

dennisbader left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dennisbader left a comment

Choose a reason for hiding this comment

id5h commented Jul 7, 2023

id5h commented Jul 7, 2023

dennisbader left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 14, 2023 •

edited by codecov bot

Loading

id5h commented Jun 19, 2023 •

edited

Loading

dennisbader left a comment •

edited

Loading

dennisbader commented Jul 3, 2023 •

edited

Loading