-
Notifications
You must be signed in to change notification settings - Fork 80
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
🚀 Deployed on https://deploy-preview-1083--etna-docs.netlify.app |
Is there is |
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 #1083 +/- ##
===========================================
- Coverage 86.51% 68.44% -18.08%
===========================================
Files 164 170 +6
Lines 8901 9304 +403
===========================================
- Hits 7701 6368 -1333
- Misses 1200 2936 +1736
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
etna/datasets/datasets_generation.py
Outdated
sigma: float = 1, | ||
random_seed: int = 1, | ||
) -> pd.DataFrame: | ||
"""Create DataFrame with hierarchical structure and AR process data. |
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.
From method name and docstring I can't understand how many levels are generated during this method.
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.
Add n_levels
parameter to make it more explicit
from etna.transforms.base import Transform | ||
|
||
|
||
class HierarchicalPipeline(Pipeline): |
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.
After all, it is somewhat confusing that HierarchicalPipeline
mimics Pipeline
, but we can't e.g. mimic the logic of AutoRegressivePipeline
or predict lower levels with ensembles.
May be it should be decorator over pipeline class that only adds logic with reconciliation.
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.
Have we thought about this possibility?
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 thought about solving it via composition, however it turns out that we should implement too much methods. But it is interesting point that levels might be predicted via different pipelines type, we haven't thought in this direction. May be we will improve this in next iteration of this track
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 should definitely think about it in the future.
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.
Look at comments above.
@@ -0,0 +1,1539 @@ | |||
{ |
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.
There are some issues with rendering of \begin{equation} y_{A,t} = y_{AA,t} + y_{AB,t} \ y_{t} = y_{A,t} + y_{B,t} \end{equation}
Reply via ReviewNB
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.
Fixed
Before submitting (must do checklist)
Proposed Changes
HierarchicaStructure
-- container to hold the hierarchical structure of the time seriesTSDataset
HierarchicalPipeline
-- pipeline to forecast time series with hierarchical structureTopDownReconciler
andBottomUpReconciler
-- hierarchical reconciliation methodsClosing issues