Update `ProphetModel` to handle external timestamp #203

d-a-bunin · 2023-12-20T15:02:25Z

Before submitting (must do checklist)

Did you read the contribution guide?
Did you update the docs? We use Numpy format for all the methods and classes.
Did you write any new necessary tests?
Did you update the CHANGELOG?

Proposed Changes

Look #186.

Closing issues

Closes #186.

…ce tests

# Conflicts: # tests/test_models/test_inference/test_forecast.py # tests/test_models/test_inference/test_predict.py

github-actions · 2023-12-20T15:07:56Z

🚀 Deployed on https://deploy-preview-203--etna-docs.netlify.app

codecov · 2023-12-20T15:17:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (unaligned-data@4f3afd5). Click here to learn what that means.

Additional details and impacted files

@@                Coverage Diff                @@
##             unaligned-data     #203   +/-   ##
=================================================
  Coverage                  ?   89.88%           
=================================================
  Files                     ?      198           
  Lines                     ?    13231           
  Branches                  ?        0           
=================================================
  Hits                      ?    11893           
  Misses                    ?     1338           
  Partials                  ?        0

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

d-a-bunin · 2023-12-21T07:45:12Z

etna/models/prophet.py

+            if not pd.api.types.is_datetime64_dtype(df[self.timestamp_column]):
+                raise ValueError("Invalid timestamp_column! Only datetime type is supported.")
+
+            if len(df[self.timestamp_column]) >= 3 and pd.infer_freq(df[self.timestamp_column]) is None:


It doesn't check that frequency is always the same. For example, it works fine if we have one frequency for train and for test. In theory, prophet can work fine even if there no regular frequency, but I'm not sure should we support this case or not.

We could probably infer frequency during train, for example, and check if it is as expected.

I think we should check the freq and give a warning if the freq for the train is different from the freq for the test.

I dont like the idea of warning. It will be thrown in every per-segment model, which seems too much.

I think it is a bad idea to have different frequencies, so the only option is to fail in this situation.

# Conflicts: # CHANGELOG.md # tests/test_models/test_inference/test_forecast.py # tests/test_models/test_inference/test_predict.py

ostreech1997 · 2023-12-25T11:50:38Z

tests/test_models/test_inference/conftest.py

Why we don't add this code to tests/conftest.py ?

For now, it seems specific for inference tests. I'll think about moving it higher.

ostreech1997 · 2023-12-25T12:38:21Z

tests/test_models/test_prophet.py

@@ -15,14 +16,26 @@
 from tests.test_models.utils import assert_sampling_is_valid


+@pytest.fixture


Code duplication (like in tests/test_models/test_inference/conftest.py)

martins0n · 2023-12-25T13:24:48Z

@d-a-bunin What do you think if Prophet would pick timestamp implicitly?
We can choose timestamp column by default. If timestamp column is of type int, we should choose the next timestamp typed column. If there are multiple timestamp typed columns - choose the first one.

And we should specify this implicit logic

d-a-bunin · 2023-12-25T13:59:14Z

Where do you think this could be useful? I can see it useful for working with pre-defined models on different datasets, e.g. in auto-ml there set of configs is pre-defined.

For now, it seems implicit, and I don't really like it. We could probably add such behavior in the future with some special parameter.
It still breaks if we have multiple timestamp columns.
We don't have such logic for DateFlagsTransform, TimeFlagsTransform and HolidayTransform.
I don't know how should we make auto-ml configs work in all those conditions.

We could make smth simple (like it works now) and easy-to-improve and then improve it according to our needs later.

martins0n · 2023-12-25T16:01:09Z

It still breaks if we have multiple timestamp columns.

No, if we choose random one

Where do you think this could be useful?

It's just simpler to support.

You've changed a lot of code here. Do we really want to change so many places just for supporting corner case?

We don't have such logic for DateFlagsTransform, TimeFlagsTransform and HolidayTransform.

Because we already have similiar beahivour for other transforms

d-a-bunin · 2023-12-26T06:34:02Z

No, if we choose random one

It works unpredictable, I don't think it is a good idea. Moreover, you suggested selecting the first one in the first message.

It's just simpler to support.

I don't think so. We have to make the same code changes + add logic for automatic column detection, which selects timestamp_column automatically instead of manually. I think that if we are going to provide some automatic selection we should also provide ability of a manual selection.

Current solution could be extended into automatic in the future by adding, e.g. parameter timestamp_mode with two possible values: "auto", "manual" with default "manual" to save current behavior.

# Conflicts: # tests/test_models/test_inference/test_forecast.py

d-a-bunin added 3 commits December 20, 2023 17:55

fix: update ProphetModel to handle external timestamp, update inferen…

7037e9d

…ce tests

Merge remote-tracking branch 'origin/unaligned-data' into issue-186

205830c

# Conflicts: # tests/test_models/test_inference/test_forecast.py # tests/test_models/test_inference/test_predict.py

fix: fix inference tests after merge

e4bf946

d-a-bunin self-assigned this Dec 20, 2023

chore: update changelog

20bdbf9

github-actions bot temporarily deployed to pull request December 20, 2023 15:07 Inactive

d-a-bunin added 2 commits December 20, 2023 19:15

fix: fix prophet doctest

00a2c34

docs: update docs for prophet model

7337903

github-actions bot temporarily deployed to pull request December 20, 2023 16:22 Inactive

d-a-bunin commented Dec 21, 2023

View reviewed changes

d-a-bunin added 2 commits December 21, 2023 18:32

Merge remote-tracking branch 'origin/unaligned-data' into issue-186

61401f0

# Conflicts: # CHANGELOG.md # tests/test_models/test_inference/test_forecast.py # tests/test_models/test_inference/test_predict.py

fix: fix after merge

8e03c56

d-a-bunin requested a review from ostreech1997 December 21, 2023 15:35

github-actions bot temporarily deployed to pull request December 21, 2023 15:38 Inactive

ostreech1997 reviewed Dec 25, 2023

View reviewed changes

d-a-bunin added 2 commits December 26, 2023 10:18

Merge remote-tracking branch 'origin/unaligned-data' into issue-186

33206b8

# Conflicts: # tests/test_models/test_inference/test_forecast.py

fix: fix after merge

a3164ed

github-actions bot temporarily deployed to pull request December 26, 2023 07:36 Inactive

fix: remove duplication of fixtures

a87986b

github-actions bot temporarily deployed to pull request December 26, 2023 08:39 Inactive

fix: fix formatting

57a17da

github-actions bot temporarily deployed to pull request December 26, 2023 08:59 Inactive

ostreech1997 approved these changes Dec 26, 2023

View reviewed changes

d-a-bunin merged commit 1e9cb9d into unaligned-data Dec 26, 2023
16 checks passed

d-a-bunin mentioned this pull request Dec 26, 2023

Update ProphetModel to handle integer timestamp #186

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `ProphetModel` to handle external timestamp #203

Update `ProphetModel` to handle external timestamp #203

d-a-bunin commented Dec 20, 2023 •

edited

github-actions bot commented Dec 20, 2023 •

edited

codecov bot commented Dec 20, 2023 •

edited

d-a-bunin Dec 21, 2023

d-a-bunin Dec 21, 2023

ostreech1997 Dec 25, 2023

d-a-bunin Dec 25, 2023

ostreech1997 Dec 25, 2023

ostreech1997 Dec 25, 2023

d-a-bunin Dec 25, 2023

ostreech1997 Dec 25, 2023

martins0n commented Dec 25, 2023

d-a-bunin commented Dec 25, 2023

martins0n commented Dec 25, 2023 •

edited

d-a-bunin commented Dec 26, 2023

		@@ -15,14 +16,26 @@
		from tests.test_models.utils import assert_sampling_is_valid


		@pytest.fixture

Update ProphetModel to handle external timestamp #203

Update ProphetModel to handle external timestamp #203

Conversation

d-a-bunin commented Dec 20, 2023 • edited

Before submitting (must do checklist)

Proposed Changes

Closing issues

github-actions bot commented Dec 20, 2023 • edited

codecov bot commented Dec 20, 2023 • edited

Codecov Report

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

martins0n commented Dec 25, 2023

d-a-bunin commented Dec 25, 2023

martins0n commented Dec 25, 2023 • edited

d-a-bunin commented Dec 26, 2023

Update `ProphetModel` to handle external timestamp #203

Update `ProphetModel` to handle external timestamp #203

d-a-bunin commented Dec 20, 2023 •

edited

github-actions bot commented Dec 20, 2023 •

edited

codecov bot commented Dec 20, 2023 •

edited

martins0n commented Dec 25, 2023 •

edited