Repository: alteryx/evalml (python)

Issue:
TimeSeriesFeaturizer doesn't appear to use correct significant lags in most cases
The lagged features that get generated by the `TimeSeriesFeaturizer` do not use the significant lags found by `_find_significant_lags` in most cases. Changing the forecast horizon and/or the gap parameters change the lagged features that are created, but these parameters are not directly used when identifying which lags are significant, only the max_delay is used.

The current process is something like this, if I am following the code correctly:
- Find all significant lags in the range of (0, max_delay)
- Add the start delay (which is gap + forecast_horizon) to the lags found from the previous step

This means that the significant lags that were identified can be offset, making them insignificant.

I believe the proper approach should be more like this:
- Use the gap and forecast_horizon and max_delay to identify the rows that fall within the feature engineering window
- Identify significant lags that fall within the feature engineering window
- Create lagged features from those significant lags without further modification

The problem is illustrated in the example below. Changing the forecast horizon changes the lagged features that are created.

**Forecast Horizon of 1**

```python
import pandas as pd
import featuretools as ft

from evalml.pipelines import TimeSeriesRegressionPipeline

df = pd.read_csv("DailyDelhiClimateTrain.csv", nrows=200)

max_delay = 10
gap = 0
forecast_horizon = 1

component_graph_as_list = ["Time Series Featurizer"]
parameters = {
    "pipeline": {
        "time_index": "date",
        "gap": gap,
        "max_delay": max_delay,
        "forecast_horizon": forecast_horizon,
    },
    "Time Series Featurizer": {
        "time_index": "date",
        "gap": gap,
        "max_delay": max_delay,
        "forecast_horizon": forecast_horizon,
    }
}
pipeline = TimeSeriesRegressionPipeline(component_graph=component_graph_as_list, parameters=parameters)

y = df.pop("meantemp")
X = df[["date"]]

p = pipeline.fit(X, y)

# For simplicity assume the first and fourth lags were found to be significant (zero indexed)
p.component_graph["Time Series Featurizer"].statistically_significant_lags = [0, 3]

p.transform(X, y).head()
```

For this first case, we create target delay features of 1 period and 4 periods as we expect
```
        date  target_delay_1  target_delay_4  target_rolling_mean
0 2013-01-01             NaN             NaN                  NaN
1 2013-01-02       10.000000             NaN                  NaN
2 2013-01-03        7.400000             NaN                  NaN
3 2013-01-04        7.166667             NaN             8.188889
4 2013-01-05        8.666667            10.0             7.744444
```

If we change nothing but making `forecast_horizon = 2` and run this again, we get a different result. We now create delay features of 2 period and 5 periods, neither of which were identified as significant. It seems to me that changing the number of points we want to forecast in the future should have no bearing on which lag features are created, only on which lags we can use.

For this case, we could not use the lag of 1 feature, because we would have no data available for the second forecast point (the feature engineering window doesn't include the previous row in this case). We should still be able to use the lag of 4 feature, which was significant.

**Forecast Horizon of 2**
```
        date  target_delay_2  target_delay_5  target_rolling_mean
0 2013-01-01             NaN             NaN                  NaN
1 2013-01-02             NaN             NaN                  NaN
2 2013-01-03       10.000000             NaN                  NaN
3 2013-01-04        7.400000             NaN                  NaN
4 2013-01-05        7.166667             NaN             8.188889
```

Pull request:
Only select lags within FE window dictated by FH and gap
Fixes #3770 

Patch:
diff --git a/docs/source/release_notes.rst b/docs/source/release_notes.rst
index 2c8f91f6..8e4c521a 100644
--- a/docs/source/release_notes.rst
+++ b/docs/source/release_notes.rst
@@ -5,6 +5,7 @@ Release Notes
     * Enhancements
         * Added fast mode to partial dependence :pr:`3753`
     * Fixes
+        * Fixed ``TimeSeriesFeaturizer`` potentially selecting lags outside of feature engineering window :pr:`3773`
         * Fixed bug where ``TimeSeriesFeaturizer`` could not encode Ordinal columns with non numeric categories :pr:`3812`
         * Updated demo dataset links to point to new endpoint :pr:`3826`
         * Updated ``STLDecomposer`` to infer the time index frequency if it's not present :pr:`3829`
diff --git a/evalml/pipelines/components/transformers/preprocessing/time_series_featurizer.py b/evalml/pipelines/components/transformers/preprocessing/time_series_featurizer.py
index a77eacc2..a8e00e17 100644
--- a/evalml/pipelines/components/transformers/preprocessing/time_series_featurizer.py
+++ b/evalml/pipelines/components/transformers/preprocessing/time_series_featurizer.py
@@ -127,6 +127,7 @@ class TimeSeriesFeaturizer(Transformer):
         self.statistically_significant_lags = self._find_significant_lags(
             y,
             conf_level=self.conf_level,
+            start_delay=self.start_delay,
             max_delay=self.max_delay,
         )
         return self
@@ -150,8 +151,8 @@ class TimeSeriesFeaturizer(Transformer):
         )
 
     @staticmethod
-    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)
         if y is not None:
             # Compute the acf and find its peaks
             acf_values, ci_intervals = acf(
@@ -161,11 +162,10 @@ class TimeSeriesFeaturizer(Transformer):
                 alpha=conf_level,
             )
             peaks, _ = find_peaks(acf_values)
-
             # Significant lags are the union of:
             # 1. the peaks (local maxima) that are significant
             # 2. The significant lags among the first 10 lags.
-            # We then filter the list to be in the range [0, max_delay]
+            # We then filter the list to be in the range [start_delay, start_delay + max_delay]
             index = np.arange(len(acf_values))
             significant = np.logical_or(ci_intervals[:, 0] > 0, ci_intervals[:, 1] < 0)
             first_significant_10 = index[:10][significant[:10]]
@@ -173,7 +173,9 @@ class TimeSeriesFeaturizer(Transformer):
                 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,
+            ]
         else:
             significant_lags = all_lags
         return significant_lags
@@ -243,10 +245,8 @@ class TimeSeriesFeaturizer(Transformer):
                 if col_name in categorical_columns:
                     col = X_categorical[col_name]
                 for t in self.statistically_significant_lags:
-                    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)
+                    feature_name = f"{col_name}_delay_{t}"
+                    lagged_features[f"{col_name}_delay_{t}"] = col.shift(t)
                     if col_name in categorical_columns:
                         cols_derived_from_categoricals.append(feature_name)
         # Handle cases where the target was passed in
@@ -254,9 +254,7 @@ class TimeSeriesFeaturizer(Transformer):
             if type(y.ww.logical_type) == logical_types.Categorical:
                 y = self._encode_y_while_preserving_index(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)
+                lagged_features[self.target_colname_prefix.format(t)] = y.shift(t)
         # Features created from categorical columns should no longer be categorical
         lagged_features = pd.DataFrame(lagged_features)
         lagged_features.ww.init(


Test patch:
diff --git a/evalml/tests/component_tests/test_time_series_featurizer.py b/evalml/tests/component_tests/test_time_series_featurizer.py
index eda1978e..2084284a 100644
--- a/evalml/tests/component_tests/test_time_series_featurizer.py
+++ b/evalml/tests/component_tests/test_time_series_featurizer.py
@@ -252,7 +252,7 @@ def test_delayed_feature_extractor_maxdelay5_forecasthorizon1_gap0(
     f"evalml.pipelines.TimeSeriesFeaturizer.{ROLLING_TRANSFORM_METHOD_NAME}",
     return_value=pd.DataFrame(),
 )
-def test_delayed_feature_extractor_maxdelay3_forecasthorizon7_gap1(
+def test_delayed_feature_extractor_maxdelay2_forecasthorizon7_gap1(
     mock_roll,
     encode_X_as_str,
     encode_y_as_str,
@@ -271,11 +271,9 @@ def test_delayed_feature_extractor_maxdelay3_forecasthorizon7_gap1(
             "feature_delay_8": X_answer.feature.shift(8),
             "feature_delay_9": X_answer.feature.shift(9),
             "feature_delay_10": X_answer.feature.shift(10),
-            "feature_delay_11": X_answer.feature.shift(11),
             "target_delay_8": y_answer.shift(8),
             "target_delay_9": y_answer.shift(9),
             "target_delay_10": y_answer.shift(10),
-            "target_delay_11": y_answer.shift(11),
         },
     )
     logical_types = _all_featurized_cols_double(answer)
@@ -283,7 +281,7 @@ def test_delayed_feature_extractor_maxdelay3_forecasthorizon7_gap1(
     assert_frame_equal(
         answer,
         TimeSeriesFeaturizer(
-            max_delay=3,
+            max_delay=2,
             forecast_horizon=7,
             gap=1,
             conf_level=1.0,
@@ -297,7 +295,6 @@ def test_delayed_feature_extractor_maxdelay3_forecasthorizon7_gap1(
             "target_delay_8": y_answer.shift(8),
             "target_delay_9": y_answer.shift(9),
             "target_delay_10": y_answer.shift(10),
-            "target_delay_11": y_answer.shift(11),
         },
     )
     logical_types = _all_featurized_cols_double(answer_only_y)
@@ -305,7 +302,7 @@ def test_delayed_feature_extractor_maxdelay3_forecasthorizon7_gap1(
     assert_frame_equal(
         answer_only_y,
         TimeSeriesFeaturizer(
-            max_delay=3,
+            max_delay=2,
             forecast_horizon=7,
             gap=1,
             conf_level=1.0,
@@ -332,14 +329,12 @@ def test_delayed_feature_extractor_numpy(mock_roll):
             1: X["date"],
             "target_delay_8": y_answer.shift(8),
             "target_delay_9": y_answer.shift(9),
-            "target_delay_10": y_answer.shift(10),
-            "target_delay_11": y_answer.shift(11),
         },
     )
     logical_types = _all_featurized_cols_double(answer)
     answer.ww.init(logical_types=logical_types)
     res = TimeSeriesFeaturizer(
-        max_delay=3,
+        max_delay=2,
         forecast_horizon=7,
         gap=1,
         conf_level=1.0,
@@ -355,8 +350,6 @@ def test_delayed_feature_extractor_numpy(mock_roll):
             "date": X["date"],
             "target_delay_8": y_answer.shift(8),
             "target_delay_9": y_answer.shift(9),
-            "target_delay_10": y_answer.shift(10),
-            "target_delay_11": y_answer.shift(11),
         },
     )
     logical_types = _all_featurized_cols_double(answer_only_y)
@@ -364,7 +357,7 @@ def test_delayed_feature_extractor_numpy(mock_roll):
     assert_frame_equal(
         answer_only_y,
         TimeSeriesFeaturizer(
-            max_delay=3,
+            max_delay=2,
             forecast_horizon=7,
             gap=1,
             conf_level=1.0,
@@ -639,11 +632,8 @@ def test_delay_feature_transformer_multiple_categorical_columns(ts_data):
         {
             "date": X["date"],
             "feature_delay_11": X_answer.feature.shift(11),
-            "feature_delay_12": X_answer.feature.shift(12),
             "feature_2_delay_11": X_answer.feature_2.shift(11),
-            "feature_2_delay_12": X_answer.feature_2.shift(12),
             "target_delay_11": y_answer.shift(11),
-            "target_delay_12": y_answer.shift(12),
         },
     )
     logical_types = _all_featurized_cols_double(answer)
@@ -838,22 +828,21 @@ def test_delayed_feature_transformer_conf_level(
 
     first_significant_10 = [l for l in significant_lags if l < 10]
     expected_lags = set(peaks).union(first_significant_10)
-    expected_lags = sorted(expected_lags.intersection(np.arange(MAX_DELAY + 1)))
+    expected_lags = sorted(
+        expected_lags.intersection(
+            np.arange(FORECAST_HORIZON, FORECAST_HORIZON + MAX_DELAY + 1),
+        ),
+    )
     answer = pd.DataFrame({"date": X["date"]})
     answer = answer.assign(
         **{
-            f"feature_delay_{t + FORECAST_HORIZON}": X["feature"].shift(
-                t + FORECAST_HORIZON,
+            f"feature_delay_{t}": X["feature"].shift(
+                t,
             )
             for t in expected_lags
         }
     )
-    answer = answer.assign(
-        **{
-            f"target_delay_{t + FORECAST_HORIZON}": y.shift(t + FORECAST_HORIZON)
-            for t in expected_lags
-        }
-    )
+    answer = answer.assign(**{f"target_delay_{t}": y.shift(t) for t in expected_lags})
     # Sort columns in alphabetical order
     answer = answer.sort_index(axis=1)
     logical_types = _all_featurized_cols_double(answer)
@@ -906,10 +895,10 @@ def test_delayed_feature_transformer_selects_first_lag_if_none_significant(
     answer = pd.DataFrame(
         {
             "date": X["date"],
-            f"feature_delay_{1 + FORECAST_HORIZON}": X["feature"].shift(
-                1 + FORECAST_HORIZON,
+            f"feature_delay_{FORECAST_HORIZON}": X["feature"].shift(
+                FORECAST_HORIZON,
             ),
-            f"target_delay_{1 + FORECAST_HORIZON}": y.shift(1 + FORECAST_HORIZON),
+            f"target_delay_{FORECAST_HORIZON}": y.shift(FORECAST_HORIZON),
         },
     )
     logical_types = _all_featurized_cols_double(answer)
diff --git a/evalml/tests/model_understanding_tests/prediction_explanations_tests/test_explainers.py b/evalml/tests/model_understanding_tests/prediction_explanations_tests/test_explainers.py
index 8b2fd6dd..0af9590c 100644
--- a/evalml/tests/model_understanding_tests/prediction_explanations_tests/test_explainers.py
+++ b/evalml/tests/model_understanding_tests/prediction_explanations_tests/test_explainers.py
@@ -1036,10 +1036,9 @@ def test_explain_predictions_time_series(ts_data):
         training_data=X_train,
         training_target=y_train,
     )
-
     # 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(
             np.array(exp["explanations"][exp_idx]["explanations"][0]["feature_values"]),
         ).any()
 
@@ -1110,8 +1109,8 @@ def test_explain_predictions_best_worst_time_series(
     if output_format == "dict":
         # Check that the computed features to be explained aren't NaN.
         for exp_idx in range(len(exp["explanations"])):
-            assert not np.isnan(
-                np.array(
+            assert not pd.isnull(
+                pd.Series(
                     exp["explanations"][exp_idx]["explanations"][0]["feature_values"],
                 ),
             ).any()