Skip to content

Forecast start_timestamp for CLI command #1265

Merged
merged 10 commits into from
May 23, 2023
Merged

Forecast start_timestamp for CLI command #1265

merged 10 commits into from
May 23, 2023

Conversation

brsnw250
Copy link
Collaborator

@brsnw250 brsnw250 commented May 18, 2023

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

Closing issues

closes #1247

@github-actions
Copy link

github-actions bot commented May 18, 2023

@github-actions github-actions bot temporarily deployed to pull request May 18, 2023 09:55 Inactive
@codecov-commenter
Copy link

codecov-commenter commented May 18, 2023

Codecov Report

Merging #1265 (b0e5c83) into master (372ee0f) will increase coverage by 0.24%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #1265      +/-   ##
==========================================
+ Coverage   87.78%   88.02%   +0.24%     
==========================================
  Files         175      186      +11     
  Lines       10439    10774     +335     
==========================================
+ Hits         9164     9484     +320     
- Misses       1275     1290      +15     
Impacted Files Coverage Δ
etna/commands/forecast_command.py 96.82% <100.00%> (+1.95%) ⬆️

... and 16 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Mr-Geekman Mr-Geekman self-requested a review May 22, 2023 07:38
CHANGELOG.md Outdated
@@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Added
- Notebook `forecast_interpretation.ipynb` with forecast decomposition ([#1220](https://github.com/tinkoff-ai/etna/pull/1220))
- Exogenous variables shift transform `ExogShiftTransform`([#1254](https://github.com/tinkoff-ai/etna/pull/1254))
-
- `start_timestamp` parameter to forecast CLI command ([#1265](https://github.com/tinkoff-ai/etna/pull/1265))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to put the word parameter at the beginning.

docs/source/commands.rst Show resolved Hide resolved
docs/source/commands.rst Show resolved Hide resolved

* :code:`prediction_interval` - whether to estimate prediction interval for forecast.
* :code:`quantiles` - levels of prediction distribution. By default 2.5% and 97.5% are taken to form a 95% prediction interval.
* :code:`n_folds` - number of folds to use in the backtest for prediction interval estimation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be we should give here a link to the class where this parameters are explained in documentation.

horizon = compute_horizon(horizon=horizon, forecast_params=forecast_params, tsdataset=tsdataset)
pipeline_configs["horizon"] = horizon # type: ignore

forecast_args = drop_additional_forecast_params(forecast_params)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It a difficult to understand naming. May be it is better to name it forecast_call_params or forecast_method_params.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant variable forecast_args, not method name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, ok)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably new name for method is also good, but it should be named get_forecast_call_params. Replace forecast_args with forecast_call_params. And update the name of the tests accordingly.

@@ -100,3 +106,112 @@ def test_forecast_use_exog_correct(
pd.testing.assert_series_equal(
df_output["target"], pd.Series(data=[3.0, 3.0, 3.0], name="target"), check_less_precise=1
)


@pytest.fixture()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually don't add () on fixtures.



@pytest.mark.parametrize("forecast_params", ({"start_timestamp": "2020-04-09"}, {"start_timestamp": "2019-04-10"}))
def test_update_horizon_error(example_tsds, forecast_params, pipeline_dummy_config):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_update_horizon_error -> test_compute_horizon_error?

({"start_timestamp": "2023-06-01"}, "ms_tsds", 4),
),
)
def test_update_horizon(forecast_params, tsdataset_name, expected, request, pipeline_dummy_config):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_update_horizon -> test_compute_horizon?

@brsnw250 brsnw250 requested a review from Mr-Geekman May 23, 2023 09:00
@github-actions github-actions bot temporarily deployed to pull request May 23, 2023 09:03 Inactive
CHANGELOG.md Outdated
@@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Added
- Notebook `forecast_interpretation.ipynb` with forecast decomposition ([#1220](https://github.com/tinkoff-ai/etna/pull/1220))
- Exogenous variables shift transform `ExogShiftTransform`([#1254](https://github.com/tinkoff-ai/etna/pull/1254))
-
- Parameter `start_timestamp` parameter to forecast CLI command ([#1265](https://github.com/tinkoff-ai/etna/pull/1265))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, the second word "parameter" should be removed.

horizon = compute_horizon(horizon=horizon, forecast_params=forecast_params, tsdataset=tsdataset)
pipeline_configs["horizon"] = horizon # type: ignore

forecast_args = drop_additional_forecast_params(forecast_params)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant variable forecast_args, not method name.

Copy link
Contributor

@Mr-Geekman Mr-Geekman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Little changes to do.

@brsnw250 brsnw250 requested a review from Mr-Geekman May 23, 2023 09:21
@github-actions github-actions bot temporarily deployed to pull request May 23, 2023 09:25 Inactive
),
),
)
def test_drop_additional_forecast_params(params, expected):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_get_forecast_call_params.

@github-actions github-actions bot temporarily deployed to pull request May 23, 2023 09:45 Inactive
@brsnw250 brsnw250 enabled auto-merge (squash) May 23, 2023 11:31
@github-actions github-actions bot temporarily deployed to pull request May 23, 2023 11:35 Inactive
@github-actions github-actions bot temporarily deployed to pull request May 23, 2023 12:21 Inactive
@brsnw250 brsnw250 merged commit 0e6d87a into master May 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Forecast start_timestamp for CLI command
3 participants