-
Notifications
You must be signed in to change notification settings - Fork 80
Add Pipeline class #78
Conversation
Codecov Report
@@ Coverage Diff @@
## master #78 +/- ##
==========================================
- Coverage 90.14% 88.14% -2.01%
==========================================
Files 48 52 +4
Lines 2152 2303 +151
==========================================
+ Hits 1940 2030 +90
- Misses 212 273 +61
Continue to review full report at Codecov.
|
etna/pipeline/pipeline.py
Outdated
self : | ||
Fitted Pipeline instance | ||
""" | ||
ts = deepcopy(ts) |
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.
May be we can use ts.copy(deep=True)
in this case.
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.
TSDataset does not have method copy
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.
In this case we do not need deep copy because we work with TSDataset (has raw_df
) and not with pd.DataFrame
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.
Looks good except for the list as default argument
etna/pipeline/pipeline.py
Outdated
class Pipeline: | ||
"""Pipeline of transforms with a final estimator.""" | ||
|
||
def __init__(self, model: Model, transforms: Iterable[Transform] = [], horizon: int = 1): |
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.
Should not use mutable objects as defaults
def __init__(self, model: Model, transforms: Iterable[Transform] = [], horizon: int = 1): | |
def __init__(self, model: Model, transforms: Iterable[Transform] = (), horizon: int = 1): |
etna/pipeline/pipeline.py
Outdated
self : | ||
Fitted Pipeline instance | ||
""" | ||
ts = deepcopy(ts) |
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.
TSDataset does not have method copy
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.
🔥
IMPORTANT: Please do not create a Pull Request without creating an issue first.
Before submitting (checklist)
make lint
(poetry install -E style
).pytest tests/
?Type of Change
Proposed Changes
-Add Pipeline class
Related Issue
PR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.
Closing issues
closes #64
Put
closes #XXXX
in your comment to auto-close the issue that your PR fixes (if such).