New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle prediction intervals in TSDataset
similar to target components
#97
Conversation
🚀 Deployed on https://deploy-preview-97--etna-docs.netlify.app |
TSDataset
similar to target components
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #97 +/- ##
==========================================
+ Coverage 89.14% 89.18% +0.04%
==========================================
Files 195 195
Lines 12554 12551 -3
==========================================
+ Hits 11191 11194 +3
+ Misses 1363 1357 -6
☔ View full report in Codecov by Sentry. |
etna/transforms/base.py
Outdated
@@ -208,7 +208,7 @@ def __init__(self, required_features: Union[Literal["all"], List[str]]): | |||
super().__init__(required_features=required_features) | |||
|
|||
@abstractmethod | |||
def _inverse_transform(self, df: pd.DataFrame) -> pd.DataFrame: | |||
def _inverse_transform(self, df: pd.DataFrame, prediction_intervals: Tuple[str, ...]) -> 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.
Can't we somehow avoid changing the signature in backward incompatible way?
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 can make this field optional, but it requires doing the same check in many different places.
But how does this make changes backward incompatible? We make this change only in internal private methods and classes that are not intended for external use cases.
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.
A small follow up, we can do it without the check at all.
The question is still relevant.
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.
Imagine that someone created their own transforms. With these changes their transforms stop working and need rework to support new way of handling intervals.
With models we still use match_target_quantiles
for extracting quantiles from adapters. Can't we make smth like this for transforms too?
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 see.
Unfortunately, we can't use match_target_quantiles
, because transforms should work with any interval borders, not only quantilies, so they might not follow name convention. In this case, we should propagate interval names from the top level (where we work with datasets). Built-in models intervals produce quantiles, that's why we can use this function there.
etna/transforms/math/limit.py
Outdated
@@ -114,13 +115,15 @@ def _transform(self, df: pd.DataFrame) -> pd.DataFrame: | |||
|
|||
return transformed_features | |||
|
|||
def _inverse_transform(self, df: pd.DataFrame) -> pd.DataFrame: | |||
def _inverse_transform(self, df: pd.DataFrame, prediction_intervals: Tuple[str, ...]) -> 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.
Doesn't we have to handle prediction intervals in this transform too?
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.
This transform handles all columns of df
similarly. Intervals are stored in df
directly, so they are transformed as well.
|
||
@pytest.fixture() | ||
def ts_with_prediction_intervals(ts_without_target_components, prediction_intervals_df): | ||
ts = deepcopy(ts_without_target_components) |
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'm not sure that it is necessary to make deepcopy
in fixtures.
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.
If we don't do so, this fixture will modify ts_without_target_components
inplace. But we want it untouched.
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 thought that fixtures doesn't interact with each other during creation.
etna/datasets/tsdataset.py
Outdated
@@ -515,9 +525,17 @@ def target_components_names(self) -> Tuple[str, ...]: | |||
return self._target_components_names | |||
|
|||
@property | |||
@deprecated( | |||
reason="Usage of this property may mislead while accessing prediction intervals. Use `prediction_intervals_names` property to access intervals names!" |
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.
Don't we want to set a version in which it is going to be removed?
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.
Idealy, it should be removed when new prediction intervals are completely stable and moved from experimental. Right now, it seems like an open question.
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 can't remove this property without major update. So, let's set version 3.0 for example for this property to be removed.
etna/transforms/math/sklearn.py
Outdated
for quantile_column_nm in quantiles: | ||
# inverse transformation of columns that are left (e.g. prediction intervals) | ||
other_columns = set(df.columns.get_level_values("feature")) - set(self.in_column) | ||
for column_nm in other_columns: |
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.
Could we use column_name
?
Before submitting (must do checklist)
Proposed Changes
Closing issues
closes #88