Only select lags within FE window dictated by FH and gap #3773
Conversation
| def _find_significant_lags(y, conf_level, max_delay): | ||
| all_lags = np.arange(max_delay + 1) | ||
| def _find_significant_lags(y, conf_level, start_delay, max_delay): | ||
| all_lags = np.arange(start_delay, start_delay + max_delay + 1) |
There was a problem hiding this comment.
only filter by lags that start in the feature engineering window.
| @@ -173,7 +174,9 @@ def _find_significant_lags(y, conf_level, max_delay): | |||
| set(index[significant]).intersection(peaks).union(first_significant_10) | |||
| ) | |||
| # If no lags are significant get the first lag | |||
| significant_lags = sorted(significant_lags.intersection(all_lags)) or [1] | |||
| significant_lags = sorted(significant_lags.intersection(all_lags)) or [ | |||
| start_delay, | |||
There was a problem hiding this comment.
default to using the start of the feature engineering window.
| @@ -246,7 +249,7 @@ def _compute_delays(self, X_ww, y): | |||
| feature_name = f"{col_name}_delay_{self.start_delay + t}" | |||
| lagged_features[ | |||
| f"{col_name}_delay_{self.start_delay + t}" | |||
| ] = col.shift(self.start_delay + t) | |||
| ] = col.shift(t) | |||
There was a problem hiding this comment.
lags already have the start delay baked in since we're only selecting lags that start from the feature engineering window.
| @@ -256,7 +259,7 @@ def _compute_delays(self, X_ww, y): | |||
| for t in self.statistically_significant_lags: | |||
| lagged_features[ | |||
| self.target_colname_prefix.format(t + self.start_delay) | |||
| ] = y.shift(self.start_delay + t) | |||
| ] = y.shift(t) | |||
There was a problem hiding this comment.
same here.
There was a problem hiding this comment.
If the start_delay is 11 and the significant lag is 11, then this comes out to a format of 21 (because 11 + 11 - 1). So we'd be shifting by 11 and the feature would be called target_delay_21? Shouldn't the shift and the column name suffix come out to the same number?
Codecov Report
@@ Coverage Diff @@
## main #3773 +/- ##
=====================================
Coverage 99.7% 99.7%
=====================================
Files 344 344
Lines 36123 36123
=====================================
Hits 35986 35986
Misses 137 137
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| lagged_features[ | ||
| f"{col_name}_delay_{self.start_delay + t}" | ||
| ] = col.shift(self.start_delay + t) | ||
| f"{col_name}_delay_{self.start_delay + t - 1}" |
There was a problem hiding this comment.
-1 since the lag is inclusive of the start delay.
| # Check that the computed features to be explained aren't NaN. | ||
| for exp_idx in range(len(exp["explanations"])): | ||
| assert not np.isnan( | ||
| assert not pd.isnull( |
There was a problem hiding this comment.
@ParthivNaresh do you know why we need to do this conversion?
jeremyliweishih
requested review from
ParthivNaresh,
eccabay and
christopherbunn
| def test_delayed_feature_extractor_maxdelay3_forecasthorizon7_gap1( | ||
| def test_delayed_feature_extractor_maxdelay2_forecasthorizon7_gap1( |
There was a problem hiding this comment.
What was the reasoning behind changing this test?
There was a problem hiding this comment.
IIRC just to match the lags on the dataset
Fixes #3770