Create assemble_pipelines 717 #774

scanhex12 · 2022-06-27T14:20:52Z

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 #717

github-actions · 2022-06-27T14:27:46Z

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

codecov-commenter · 2022-06-28T14:29:35Z

Codecov Report

Merging #774 (77cec5b) into master (f1befc5) will increase coverage by 0.07%.
The diff coverage is 97.43%.

@@            Coverage Diff             @@
##           master     #774      +/-   ##
==========================================
+ Coverage   83.80%   83.87%   +0.07%     
==========================================
  Files         123      124       +1     
  Lines        6902     6941      +39     
==========================================
+ Hits         5784     5822      +38     
- Misses       1118     1119       +1

Impacted Files	Coverage Δ
etna/pipeline/assembling_pipelines.py	`97.36% <97.36%> (ø)`
etna/pipeline/__init__.py	`100.00% <100.00%> (ø)`

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

…mble_pipelines

… into assemble_pipelines

iKintosh

Code looks good, however it does not work as stated in the issue description.

It should match not only model-horizon, but transforms-models in case if we want different transform for different models. The example of this behaviour could be seen in the issue description.

Important note: if we want to skip transform for the model we use None instead

assemble_pipelines([Catboost(), Prophet()], [TrendTransform(in_column="target"), [LagTransform(lags=[1,2,3], in_column="target"), **None**], [1, 2])

Pipeline(model=Catboost(), transforms=[TrendTransform(in_column="target"), LagTransform(lags=[1,2,3], in_column="target")], horizon=1)
Pipeline(model=Prophet(), transforms=[TrendTransform(in_column="target")], horizon=2)

Also there are not enough tests.
You should add test case if we have different models for different horizons, as well as different transforms for different models.

iKintosh · 2022-07-05T07:27:15Z

etna/pipeline/assemble_pipelines.py

+
+def assemble_pipelines(
+    models: Union[BaseModel, Sequence[BaseModel]],
+    transforms: Sequence[Transform],


It should be able to work with cases such as

assemble_pipelines(Catboost(), [TrendTransform(in_column="target"), [LagTransform(lags=[1,2,3], in_column="target"), LagTransform(lags=[2,3,4], in_column="target")]], [1, 2])

And as a result it should create the following two pipelines:

Pipeline(model=Catboost(), transforms=[TrendTransform(in_column="target"), LagTransform(lags=[1,2,3], in_column="target")], horizon=1) Pipeline(model=Catboost(), transforms=[TrendTransform(in_column="target"), LagTransform(lags=[2,3,4], in_column="target")], horizon=2)

iKintosh · 2022-07-05T07:27:53Z

tests/test_pipeline/test_assemble_pipelines.py

+from etna.transforms import TrendTransform
+
+
+def test_not_equal_lengths():


Good idea for test! 👍

Mr-Geekman · 2022-07-13T08:09:40Z

etna/pipeline/assemble_pipelines.py

+    horizons:
+        Sequence of horizons
+
+    Raises


I think we should swap raises and returns blocks and add empty line between them.

Mr-Geekman · 2022-07-14T08:05:18Z

etna/pipeline/__init__.py

@@ -1,3 +1,4 @@
 from etna.pipeline.autoregressive_pipeline import AutoRegressivePipeline
 from etna.pipeline.base import FoldMask
 from etna.pipeline.pipeline import Pipeline
+from etna.pipeline.pipelines_fabric import assemble_pipelines


fabric -- is a material to make clothes. Try smth like "assmbling_pipelines" for file name.

pipeline_assembling

Mr-Geekman · 2022-07-14T09:02:30Z

etna/pipeline/assmbling_pipelines.py

+    transforms: Sequence[Union[Transform, Sequence[Optional[Transform]]]],
+    horizons: Union[int, Sequence[int]],
+) -> List[Pipeline]:
+    """Create pipelines from input horizons and models or sequence of models.


May be just:
Create pipelines with broadcasting from models, transforms and horizons.

Mr-Geekman · 2022-07-14T09:04:17Z

etna/pipeline/assmbling_pipelines.py

+    Parameters
+    ----------
+    models:
+        Instance or Sequence of the etna Model


Instance of Sequence of models

Mr-Geekman · 2022-07-14T09:04:40Z

etna/pipeline/assmbling_pipelines.py

+    models:
+        Instance or Sequence of the etna Model
+    transforms:
+        Sequence of the transforms


I think we should explain here how broadcasting is made. It is not obvious.

Mr-Geekman · 2022-07-14T09:05:11Z

etna/pipeline/assmbling_pipelines.py

+    horizons: Union[int, Sequence[int]],
+) -> List[Pipeline]:
+    """Create pipelines from input horizons and models or sequence of models.
+


Or we should add description about broadcasting here.

iKintosh

Looks good. Ready to merge. Fixed import errors.

iKintosh · 2022-07-14T09:15:16Z

etna/pipeline/assembling_pipelines.py

+    ValueError:
+        If the length of models sequence not equals to length of horizons sequence.
+
+    Examples


👍
This is a good practice

ext.kvedernikov added 2 commits June 27, 2022 17:19

ADD: assemble pipeline

253fe38

CHANGE: change path to pipelines

4e77bdc

github-actions bot temporarily deployed to pull request June 27, 2022 14:27 Inactive

CHANGE: delete examples

8e1e2ca

github-actions bot temporarily deployed to pull request June 28, 2022 07:32 Inactive

ADD: changelog, CHANGE: docstrings of method

17f59de

github-actions bot temporarily deployed to pull request June 28, 2022 08:34 Inactive

iKintosh self-requested a review June 28, 2022 11:45

CHANGE: update poetry

c305a49

github-actions bot temporarily deployed to pull request June 28, 2022 13:44 Inactive

ext.kvedernikov and others added 2 commits July 1, 2022 12:50

CHANGE: poetry lock

43d38c9

Merge branch 'master' into assemble_pipelines

1b16f20

github-actions bot temporarily deployed to pull request July 1, 2022 09:58 Inactive

ext.kvedernikov added 2 commits July 1, 2022 13:20

Merge branch 'master' of https://github.com/tinkoff-ai/etna into asse…

3127fcf

…mble_pipelines

Merge branch 'assemble_pipelines' of https://github.com/tinkoff-ai/etna…

e557cc2

… into assemble_pipelines

iKintosh suggested changes Jul 5, 2022

View reviewed changes

CHANGE: behavoir of assemble pipelines, tests

8be71ef

github-actions bot temporarily deployed to pull request July 5, 2022 23:45 Inactive

iKintosh self-requested a review July 6, 2022 12:58

CHANGE: function behavoir, tests

2193692

scanhex12 force-pushed the assemble_pipelines branch from 5a46015 to 2193692 Compare July 7, 2022 13:44

github-actions bot temporarily deployed to pull request July 7, 2022 13:52 Inactive

CHANGE: function implementation

2036630

scanhex12 force-pushed the assemble_pipelines branch from c3f97d8 to 2036630 Compare July 7, 2022 14:12

Merge branch 'master' into assemble_pipelines

bc0b331

github-actions bot temporarily deployed to pull request July 7, 2022 14:18 Inactive

martins0n requested a review from Mr-Geekman July 13, 2022 07:44

Mr-Geekman reviewed Jul 13, 2022

View reviewed changes

CHANGE: poetry

f6be3be

github-actions bot temporarily deployed to pull request July 14, 2022 07:32 Inactive

scanhex12 requested review from iKintosh and Mr-Geekman July 14, 2022 07:52

Mr-Geekman reviewed Jul 14, 2022

View reviewed changes

ext.kvedernikov added 2 commits July 14, 2022 11:42

CHANGE: review changes

cac0fe8

CHANGE: review

7faa844

github-actions bot temporarily deployed to pull request July 14, 2022 08:46 Inactive

Mr-Geekman reviewed Jul 14, 2022

View reviewed changes

CHANGE: return doctring

68edf7f

iKintosh previously approved these changes Jul 14, 2022

View reviewed changes

github-actions bot temporarily deployed to pull request July 14, 2022 09:33 Inactive

ext.kvedernikov added 2 commits July 14, 2022 13:05

CHANGE: docstrings

b623823

CHANGE: docstrings

9810481

scanhex12 dismissed iKintosh’s stale review via 9810481 July 14, 2022 10:21

scanhex12 force-pushed the assemble_pipelines branch from 0906411 to 9810481 Compare July 14, 2022 10:21

github-actions bot temporarily deployed to pull request July 14, 2022 10:24 Inactive

CHANGE: docstring

16d5779

github-actions bot temporarily deployed to pull request July 14, 2022 10:30 Inactive

CHANGE: docstring

adf73d3

github-actions bot temporarily deployed to pull request July 14, 2022 10:37 Inactive

CHANGE: math docstrings

702647e

github-actions bot temporarily deployed to pull request July 14, 2022 10:59 Inactive

CHANGE: math docstrings.

77cec5b

github-actions bot temporarily deployed to pull request July 14, 2022 11:05 Inactive

Mr-Geekman approved these changes Jul 14, 2022

View reviewed changes

scanhex12 merged commit ea68310 into master Jul 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create assemble_pipelines 717 #774

Create assemble_pipelines 717 #774

scanhex12 commented Jun 27, 2022

github-actions bot commented Jun 27, 2022 •

edited

Loading

codecov-commenter commented Jun 28, 2022 •

edited

Loading

iKintosh left a comment •

edited

Loading

iKintosh Jul 5, 2022

iKintosh Jul 5, 2022

Mr-Geekman Jul 13, 2022

Mr-Geekman Jul 14, 2022

iKintosh Jul 14, 2022

Mr-Geekman Jul 14, 2022

Mr-Geekman Jul 14, 2022

Mr-Geekman Jul 14, 2022

Mr-Geekman Jul 14, 2022

iKintosh left a comment

iKintosh Jul 14, 2022

		from etna.transforms import TrendTransform


		def test_not_equal_lengths():

Create assemble_pipelines 717 #774

Create assemble_pipelines 717 #774

Conversation

scanhex12 commented Jun 27, 2022

Before submitting (must do checklist)

Proposed Changes

Closing issues

github-actions bot commented Jun 27, 2022 • edited Loading

codecov-commenter commented Jun 28, 2022 • edited Loading

Codecov Report

iKintosh left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iKintosh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 27, 2022 •

edited

Loading

codecov-commenter commented Jun 28, 2022 •

edited

Loading

iKintosh left a comment •

edited

Loading