-
Notifications
You must be signed in to change notification settings - Fork 80
Add target components logic to TSDataset
#1153
Conversation
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 #1153 +/- ##
==========================================
+ Coverage 86.39% 86.93% +0.53%
==========================================
Files 166 177 +11
Lines 9513 9847 +334
==========================================
+ Hits 8219 8560 +341
+ Misses 1294 1287 -7
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
🚀 Deployed on https://deploy-preview-1153--etna-docs.netlify.app |
etna/datasets/tsdataset.py
Outdated
@@ -464,6 +467,11 @@ def regressors(self) -> List[str]: | |||
""" | |||
return self._regressors | |||
|
|||
@property | |||
def target_components(self) -> Optional[List[str]]: | |||
"""Get list of target components. Target components sum up to target.""" |
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.
I think we should write that if there are no components, None
is returned.
etna/datasets/tsdataset.py
Outdated
if self._target_components is not None: | ||
raise ValueError("Dataset already contains target components!") | ||
|
||
components_names = set(target_components_df.columns.get_level_values("feature")) |
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.
What if there is duplication in names of components?
Should we do something with |
tests/test_datasets/test_dataset.py
Outdated
|
||
def test_add_target_components(ts_without_target_components, ts_with_target_components, target_components_df): | ||
ts_without_target_components.add_target_components(target_components_df=target_components_df) | ||
pd.testing.assert_frame_equal(ts_with_target_components.to_pandas(), ts_with_target_components.to_pandas()) |
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.
Probably should be:
pd.testing.assert_frame_equal(ts_without_target_components.to_pandas(), ts_with_target_components.to_pandas()
@@ -481,3 +481,11 @@ def test_get_level_dataset_lower_level_error(simple_hierarchical_ts): | |||
def test_get_level_dataset_with_quantiles(product_level_constant_forecast_w_quantiles, target_level, answer): | |||
forecast = product_level_constant_forecast_w_quantiles | |||
np.testing.assert_array_almost_equal(forecast.get_level_dataset(target_level=target_level).df.values, answer) | |||
|
|||
|
|||
def test_get_level_dataset_pass_target_components_to_output(simple_hierarchical_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.
Can you please explain what is happening here and why?
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.
Here I test that get_level_dataset pass target_components to the output dataset
It should be done as get_level_dataset creates new dataset with the same columns but different set of segments
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.
What should we do with it? |
etna/datasets/tsdataset.py
Outdated
for segment in self.segments: | ||
components_names_segment = sorted(target_components_df[segment].columns.get_level_values("feature")) | ||
if components_names != components_names_segment: | ||
raise ValueError("Set of target components differs between segments!") |
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 we write between which segments there is a difference?
Before submitting (must do checklist)
Proposed Changes
Closing issues
closes #1142