-
Notifications
You must be signed in to change notification settings - Fork 80
Add forecast components handling to base classes of models #1158
Conversation
🚀 Deployed on https://deploy-preview-1158--etna-docs.netlify.app |
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #1158 +/- ##
==========================================
+ Coverage 86.60% 86.91% +0.30%
==========================================
Files 166 177 +11
Lines 9555 9925 +370
==========================================
+ Hits 8275 8626 +351
- Misses 1280 1299 +19
... and 13 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
tests/test_models/test_mixins.py
Outdated
) | ||
@pytest.mark.parametrize("return_components", (True, False)) | ||
def test_model_mixins_calls_add_target_components_in_forecast(mixin_constructor, return_components, call_params): | ||
with patch.multiple(mixin_constructor, __abstractmethods__=set()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is it for? Looks very suspicious.
tests/test_models/test_mixins.py
Outdated
|
||
|
||
@pytest.mark.parametrize("mixin_constructor", [PerSegmentModelMixin]) | ||
def test_make_prediction_segment_with_components(mixin_constructor, target_components_df, ts_without_target_components): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't changed _make_predictions_segment
in this task, do we really need this test?
Should we add other tests on this method as well (no components, handling prediction_size
, etc)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we check how it works with target components
tests/test_models/test_mixins.py
Outdated
from etna.models.mixins import SaveNNMixin | ||
|
||
|
||
class DummyPredictAdapter(BaseAdapter): | ||
def fit(self, df: pd.DataFrame, **kwargs) -> "DummyAdapter": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typing is incorrect
tests/test_models/test_mixins.py
Outdated
|
||
|
||
class DummyForecastPredictAdapter(DummyPredictAdapter): | ||
def fit(self, df: pd.DataFrame, **kwargs) -> "DummyAdapter": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typing is incorrect
Before submitting (must do checklist)
Proposed Changes
Closing issues
closes #1124