Repository: alan-turing-institute/sktime
Pull request #3261 (2022-08-13 19:32:58)

[ENH] in `Lag`, make column naming consistent between single-lag and multi-lag case
Problem:
`Lag` outputs column names prefixed with "lag_x__" when using multiple lags. If a single lag is specified then the output column name is the same as the original column name (if the input is a `pd.DataFrame` rather than a `pd.Series`) without "lag_x__" prefixed to the original column name. It would be desirable for the output column names to be consistent, irrespective if a single lag or multiple lags is specified.

Fix implemented:
`Lag` now outputs the same column name irrespective of whether a single lag or multiple lags is specified.

KishManani: The context which drove me to want to make a change was related to creating a pipeline of transformers. I have a pipeline where I want to apply the `Lag` transformer to different columns with different lags, in some cases I want to apply a single lag to a given column like so:

```
feat_eng_pipe = ColumnTransformer(
                  transformers=[
                    ('', 'passthrough', ['x1']),
                    ('lag_x2', Lag(lags=[3]), ['x2']),
                    ('lag_x3', Lag(lags=[1,2,3]), ['x3']),
                    ],
                  remainder='drop',
                  preserve_dataframe=True
                  )
```

The resulting dataframe from applying this pipeline will give different column names to lag variables created from `x2` compared to `x3`. This inconsistency is troublesome because I will now have a dataframe where the lag variables of `x3` are prefixed with `lag_{}__` but the lag variable of `x2` will just be called `x2` which could be mistaken for just passing through `x2`. Note, in this example because no two transformers act on the same column we do not get the transformer names being prefixed to the output columns.

> Re code:
If you want to remove the case distinction, I think it would be possible more easily, by just removing the "if" branch of the "if/else", and running only the "else" part?

Yes you're right! I've made the change and also added a unit test. It would be good to get your feedback on the unit test also if you think it looks a bit verbose.
 
fkiraly: >This inconsistency is troublesome

I see, I'm convinced now - the inconsistency is a frustrated user expectation bordering on being a bug, in this case.

> I've made the change and also added a unit test. It would be good to get your feedback on the unit test also if you think it looks a bit verbose.

The new test looks good to me.

Planning to approve/merge once/if the CI suite passes.
fkiraly: PS: since 0.13.1, you should be able to do sth like this:

```python
feat_eng_pipe = Lag()['x1'] + Lag(lags=[3])['x2'] + Lag(lags=[1,2,3])['x3']
```
where the square brackets apply the transformer only to those variables, and the addition means feature union.

Optional passthrough is not yet a dunder, but was thinking of the tilde.
KishManani: > PS: since 0.13.1, you should be able to do sth like this:
> 
> ```python
> feat_eng_pipe = Lag()['x1'] + Lag(lags=[3])['x2'] + Lag(lags=[1,2,3])['x3']
> ```
> 
> where the square brackets apply the transformer only to those variables, and the addition means feature union.
> 
> Optional passthrough is not yet a dunder, but was thinking of the tilde.

Thanks! Very cool! Will be using this in the future. Tilde sounds very reasonable. An alternative would be to implement an identity transformer if you wanted a more explicit, but more verbose, syntax.
fkiraly: > Thanks! Very cool! Will be using this in the future. Tilde sounds very reasonable. An alternative would be to implement an identity transformer if you wanted a more explicit, but more verbose, syntax.

Say, what do you think of the variable naming that happens in this case? I think you get a double prefix with `Lag`, once for the transformer, once for the lag. That looks a bit odd.

Patch:
diff --git a/sktime/transformations/series/lag.py b/sktime/transformations/series/lag.py
index 1fa39932f..88d7644b3 100644
--- a/sktime/transformations/series/lag.py
+++ b/sktime/transformations/series/lag.py
@@ -259,13 +259,10 @@ class Lag(BaseTransformer):
 
             Xt_list.append(Xt)
 
-        if len(Xt_list) == 1:
-            Xt = Xt_list[0]
-        else:
-            lag_names = self._yield_shift_param_names()
-            Xt = pd.concat(Xt_list, axis=1, keys=lag_names, names=["lag", "variable"])
-            if self.flatten_transform_index:
-                Xt.columns = flatten_multiindex(Xt.columns)
+        lag_names = self._yield_shift_param_names()
+        Xt = pd.concat(Xt_list, axis=1, keys=lag_names, names=["lag", "variable"])
+        if self.flatten_transform_index:
+            Xt.columns = flatten_multiindex(Xt.columns)
 
         # some pandas versions do not sort index automatically after concat
         # so removing will break specific pandas versions


Test patch:
diff --git a/sktime/transformations/series/tests/test_featureizer.py b/sktime/transformations/series/tests/test_featureizer.py
index 53a00eb38..1f7149e1b 100644
--- a/sktime/transformations/series/tests/test_featureizer.py
+++ b/sktime/transformations/series/tests/test_featureizer.py
@@ -28,4 +28,4 @@ def test_featurized_values():
     exp_transformer = ExponentTransformer()
     expected_len = lags + len(y_test)
     y_hat = exp_transformer.fit_transform(y[-expected_len:])
-    assert_array_equal(X_hat["TOTEMP"].values, y_hat.values)
+    assert_array_equal(X_hat[f"lag_{lags}__TOTEMP"].values, y_hat.values)
diff --git a/sktime/transformations/series/tests/test_lag.py b/sktime/transformations/series/tests/test_lag.py
index 02819643b..e496f72db 100644
--- a/sktime/transformations/series/tests/test_lag.py
+++ b/sktime/transformations/series/tests/test_lag.py
@@ -4,6 +4,8 @@
 
 __author__ = ["fkiraly"]
 
+import itertools
+
 import pandas as pd
 import pytest
 
@@ -63,3 +65,31 @@ def test_lag_fit_transform_columns(X, index_out, lag):
             return 1
 
     assert ncols(Xt) == ncols(X) * len_lag
+
+
+@pytest.mark.parametrize("X", X_fixtures)
+@pytest.mark.parametrize("index_out", index_outs)
+@pytest.mark.parametrize("lags", [2, [2, 4]])
+def test_lag_fit_transform_column_names(X, index_out, lags):
+    """Test expected column names."""
+    t = Lag(lags=lags, index_out=index_out)
+    Xt = t.fit_transform(X)
+
+    if isinstance(Xt, pd.DataFrame):
+        lag_col_names = set(Xt.columns)
+
+        if isinstance(X, pd.DataFrame):
+            col_names = X.columns
+        elif isinstance(X, pd.Series):
+            col_names = [X.name if X.name else 0]
+        else:
+            pass
+
+        lags = [lags] if isinstance(lags, int) else lags
+        expected = {
+            f"lag_{lag}__{col}" for lag, col in itertools.product(lags, col_names)
+        }
+        assert lag_col_names == expected
+
+    elif isinstance(Xt, pd.Series):
+        assert Xt.name is None