Implement predict in Pipeline, AutoRegressivePipeline
#970
Conversation
…s for predict method in Pipeline, AutoRegressivePipeline
|
🚀 Deployed on https://deploy-preview-970--etna-docs.netlify.app |
Codecov Report
@@ Coverage Diff @@
## inference #970 +/- ##
============================================
Coverage ? 49.87%
============================================
Files ? 132
Lines ? 7673
Branches ? 0
============================================
Hits ? 3827
Misses ? 3846
Partials ? 0 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
etna/pipeline/mixins.py
Outdated
| from etna.transforms import Transform | ||
|
|
||
|
|
||
| class PipelineModelPredictMixin: |
There was a problem hiding this comment.
Can we add all this functions to BasePipeline?
There was a problem hiding this comment.
What for? They aren't needed for ensembles, only for Pipeline and AutoRegressivePipeline?
There was a problem hiding this comment.
But the naming misleading PipelineModelPredictMixin, looks like it for all Pipelines
There was a problem hiding this comment.
How would you name it? Is ModelPipelinePredictMixin sounds ok?
There was a problem hiding this comment.
I don't know. ModelPipelinePredictMixin is the same as PipelineModelPredictMixin. lol
I don't understand why I shouldn't use it in ensembles.
tests/test_pipeline/test_mixins.py
Outdated
| [DateFlagsTransform(), FilterFeaturesTransform(exclude=["regressor_exog_weekend"])], | ||
| ], | ||
| ) | ||
| def test_prepare_ts(context_size, start_idx, end_idx, transforms, example_reg_tsds): |
There was a problem hiding this comment.
It would be more obvious to specify expected values by hand: now expected values computed in runtime -- I guess it's a bit error prone solution: you use the same code to compute expected values - it helps to find bugs in regression testing but code could already has logical errors.
There was a problem hiding this comment.
Do you suggest to use start_timestamp and end_timestamp here instead of start_idx and end_idx?
There was a problem hiding this comment.
Or add expected_start_idx here?
There was a problem hiding this comment.
start_timestamp and end_timestamp
Yep
I guess we know them without any computation
| quantiles=[], | ||
| ) | ||
|
|
||
| expected_ts = mixin._recreate_ts.return_value |
There was a problem hiding this comment.
exprcted_ts from return_value?
There was a problem hiding this comment.
Method _recreate_ts is tested. So in this test I check that on the result of calling _recreate_ts the transforms are applied as expected.
There was a problem hiding this comment.
It doesn't make more sense to me.But it's minor issue that could be ignored
etna/pipeline/base.py
Outdated
| ) -> TSDataset: | ||
| raise NotImplementedError() | ||
| @staticmethod | ||
| def _validate_predict_timestamps( |
There was a problem hiding this comment.
This is not validation I guess
There was a problem hiding this comment.
Why not? It checks that it is in appropriate range and fills the value if it isn't present.
There was a problem hiding this comment.
It looks like builder creator or something like this
Before submitting (must do checklist)
Proposed Changes
Look #958.
Closing issues
Closes #958.