Shift of exogenous data #1254
Shift of exogenous data #1254
Conversation
|
🚀 Deployed on https://deploy-preview-1254--etna-docs.netlify.app |
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #1254 +/- ##
==========================================
+ Coverage 87.88% 87.99% +0.11%
==========================================
Files 186 186
Lines 10649 10749 +100
==========================================
+ Hits 9359 9459 +100
Misses 1290 1290
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
etna/transforms/math/lags.py
Outdated
| The fitted transform instance. | ||
| """ | ||
| df_exog = ts.df_exog | ||
| if df_exog is not None and isinstance(df_exog, pd.DataFrame): |
There was a problem hiding this comment.
Why do we need isinstance check here?
There was a problem hiding this comment.
Can't we put all the necessary logic of checking isinstance inside of _save_exog_last_date? I think we should also set self._exog_last_date = {} if there are no exogs.
There was a problem hiding this comment.
This check was needed for the base class. Now it seems irrelevant.
etna/transforms/math/lags.py
Outdated
| if not self._auto: | ||
| return self.lag # type: ignore | ||
|
|
||
| freq = pd.infer_freq(df.index) |
There was a problem hiding this comment.
Can't we somehow get freq from ts.freq?
There was a problem hiding this comment.
We can save this frequency in fit method and reuse it here.
There was a problem hiding this comment.
You are also using frequency in transform.
There was a problem hiding this comment.
To work, this transform needs similar frequency both on fit and transform.
etna/transforms/math/lags.py
Outdated
| raise ValueError("Transform is not fitted!") | ||
|
|
||
| result = df | ||
| freq = pd.infer_freq(df.index) |
etna/transforms/math/lags.py
Outdated
| """ | ||
| df_exog = ts.df_exog | ||
| if df_exog is not None and isinstance(df_exog, pd.DataFrame): | ||
| self._save_exog_last_date(df_exog=df_exog) |
There was a problem hiding this comment.
Why are we passing df_exog here? We can only shift something that was in df_exog?
There was a problem hiding this comment.
Basically, the use case of this transform is to shift additional regressors when they are not available all the way down to the horizon. Is this transform necessary in case of other types of features?
There was a problem hiding this comment.
Ok, I think we will save us from some trouble if we will keep working only with df_exog.
The problem that we can have quantiles in df_exog in theory. It shouldn't be possible but it can work like this right now...
I think we can ingore this problem in this transform for now. It should be fixed on the level of handling quantiles.
There was a problem hiding this comment.
You could put logic with checking None inside of self._save_exog_last_date.
etna/transforms/math/lags.py
Outdated
| """ | ||
| df_exog = ts.df_exog | ||
| if df_exog is not None and isinstance(df_exog, pd.DataFrame): | ||
| self._save_exog_last_date(df_exog=df_exog) |
There was a problem hiding this comment.
You could put logic with checking None inside of self._save_exog_last_date.
etna/transforms/math/lags.py
Outdated
| feature_names = list(self._exog_last_date.keys()) | ||
|
|
||
| else: | ||
| feature_names = [] |
There was a problem hiding this comment.
Could we really reach this else clause?
There was a problem hiding this comment.
Yes, for example when dataset has no exog varibales.
There was a problem hiding this comment.
Can we rewrite it like this?
if self._exog_last_date is not None:
feature_names = list(self._exog_last_date.keys())
else:
feature_names = []Because we have a guarantee that if self._exog_last_date isn't None it is dict.
It also can't be called with self._exog_last_date is None unless it is called directly.
Before submitting (must do checklist)
Proposed Changes
Closing issues
closes #1234