Fix TSDataset.make_future to handle hierarchy, quantiles, target components
#1248
Conversation
Mr-Geekman
changed the title
make_future to handle hierarchy, quantiles, target componentsTSDataset.make_future to handle hierarchy, quantiles, target components
|
🚀 Deployed on https://deploy-preview-1248--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 #1248 +/- ##
==========================================
- Coverage 87.88% 87.86% -0.02%
==========================================
Files 186 186
Lines 10639 10643 +4
==========================================
+ Hits 9350 9352 +2
- Misses 1289 1291 +2
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -295,6 +297,13 @@ def make_future( | |||
| f"NaN-s will be used for missing values" | |||
| ) | |||
|
|
|||
| # remove components and quantiles | |||
There was a problem hiding this comment.
Both of them are not added to raw_df, not sure that we really need to drop the explicitly
There was a problem hiding this comment.
In our fixtures we create some datasets, giving components and quantiles in raw_df. For example, you can look at fixtures that was used in test_make_future_removes_quantiles and test_make_future_removes_target_components. We can try to fix this fixtures, I think.
For components we can in some sense guarantee that there will be no components (because they are added using special method). But for quantiles we can't make such guarantee.
There was a problem hiding this comment.
I think a good solution should consist of two steps:
- Rework quantiles to be like components
- Fix our fixtures to follow the procedure of adding quantiles and components
We can probably create some task for this.
Offtop, but I also think that we should somehow make a more formal restriction on columns that can be passed into df of TSDataset.__init__. Because we suggest that it is only endog, but we don't check this and in a lot of inner code don't follow this rule, so it can lead to confusing situations.
There was a problem hiding this comment.
Agree, the right solution is to rework quantiles. What kind of restrictions do you mean? Like allow only target?
There was a problem hiding this comment.
What are we going to do with this issue? Leave dropping this way and fix it after fixing quantiles?
There was a problem hiding this comment.
We should discuss do we want this restriction or not, but the proposal is to allow in df to be only columns timestamp, segment, target. We can also generate a good error message if smth went wrong.
There was a problem hiding this comment.
What are we going to do with this issue? Leave dropping this way and fix it after fixing quantiles?
Probably, we can do like this, yes. This piece of code won't break anything (as I understand), but can safe us for now from some strange moments. We can add todo about fixing it after reworking quantiles. But we have a problem that we aren't really fixing todos. We can create a task on fixing quantiles and point out in that task to remove these redundant lines.
There was a problem hiding this comment.
- Let's leave TODO
- We can restrict df to have only timestamp, segment, target -- I guess it should prevent people adding exogs incorrectly. However it seems that in some places we create dataset out of df with extra columns ourselvs
Before submitting (must do checklist)
Proposed Changes
Look #1244.
Closing issues
Closes #1244.