Unification of errors, warnings and checks in models #1312
Conversation
…utoARIMAModel, SARIMAXModel about forecast, predict methods, change exception types and messages
etna/models/autoarima.py
Outdated
| @@ -57,7 +57,10 @@ class AutoARIMAModel( | |||
| """ | |||
| Class for holding auto arima model. | |||
|
|
|||
| Method ``predict`` can use true target values only on train data on future data autoregression | |||
| Method ``forecast`` can be used on ouf-of-sample data that goes after training data with a gap. | |||
There was a problem hiding this comment.
I'm not really sure about adding this clarifications into documentation.
There was a problem hiding this comment.
Maybe we should specify all cases in one place for all models.
And to be clear we should make illustrations because current docs really hard to understand
There was a problem hiding this comment.
Ok, let's do this in some other task. May be it better fits the documentation track.
| @@ -112,10 +112,11 @@ def fit(self, ts: TSDataset) -> "DeadlineMovingAverageModel": | |||
| self._freq = freq | |||
|
|
|||
| columns = set(ts.columns.get_level_values("feature")) | |||
| if columns != {"target"}: | |||
There was a problem hiding this comment.
May be some common blocks should be extracted somewhere for all models.
|
🚀 Deployed on https://deploy-preview-1312--etna-docs.netlify.app |
Codecov Report
❗ 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 #1312 +/- ##
==========================================
- Coverage 88.94% 6.69% -82.25%
==========================================
Files 203 203
Lines 12466 12498 +32
==========================================
- Hits 11088 837 -10251
- Misses 1378 11661 +10283
... and 177 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
etna/models/autoarima.py
Outdated
| @@ -57,7 +57,10 @@ class AutoARIMAModel( | |||
| """ | |||
| Class for holding auto arima model. | |||
|
|
|||
| Method ``predict`` can use true target values only on train data on future data autoregression | |||
| Method ``forecast`` can be used on ouf-of-sample data that goes after training data with a gap. | |||
There was a problem hiding this comment.
Maybe we should specify all cases in one place for all models.
And to be clear we should make illustrations because current docs really hard to understand
| @@ -156,7 +172,10 @@ def predict_components(self, df: pd.DataFrame) -> pd.DataFrame: | |||
| raise ValueError("Model is not fitted! Fit the model before estimating forecast components!") | |||
|
|
|||
| if self._last_train_timestamp < df["timestamp"].max() or self._first_train_timestamp > df["timestamp"].min(): | |||
| raise ValueError("To estimate out-of-sample prediction decomposition use `forecast` method.") | |||
| raise NotImplementedError( | |||
There was a problem hiding this comment.
What do you think about module with common Exceptions and warnings?
There was a problem hiding this comment.
I think it is a reasonable way to go.
Do we want to do this in this task? Or it is better to do this in some other task? Because it can require more planning than changing error messages like we do in this task.
There was a problem hiding this comment.
You can make it in another task if some research is needed
Before submitting (must do checklist)
Proposed Changes
Unify errors, warnings and checks in multiple models.
Closing issues