Skip to content

fix(temporal): emit feature names in the order transform produces - #27

Open
ChrisW09 wants to merge 1 commit into
mainfrom
fix/temporal-feature-name-order
Open

fix(temporal): emit feature names in the order transform produces#27
ChrisW09 wants to merge 1 commit into
mainfrom
fix/temporal-feature-name-order

Conversation

@ChrisW09

Copy link
Copy Markdown
Collaborator

Fixes #15.

Problem

LagFeatureTransformer, RollingStatsTransformer and CyclicalTimeTransformer all build
their output by hstacking one (n_rows, n_features) block per lag / statistic / component,
so the columns are block-major. But they inherit
BasePreTabTransformer.get_feature_names_out, which walks
zip(input_features, self._output_sizes()) and emits feature-major names.

For any input with more than one column, the labels are simply wrong:

Lag  names: ['A_lag0', 'A_lag1', 'B_lag0', 'B_lag1']
Lag  row 0: [  1. 100.   0.   0.]     # col 1 = 100 is B's lag-1, labelled A_lag1
Roll row 0: [  1. 100.   2. 200.]     # actual order mean_A, mean_B, max_A, max_B

Single-column input happens to be correct, which is why the existing tests pass.

Fix

Override get_feature_names_out in each of the three classes to iterate blocks outermost,
matching the actual column layout.

Scope note

I kept the existing lag{j} / roll{j} / cyclic{j} suffix scheme rather than renaming to
_mean / _sin / _cos. The issue suggested that rename as a bonus and it is genuinely
more informative — but it changes public output names for single-feature users whose labels
are correct today, which is a different kind of change from fixing a mislabelling. Keeping
the suffixes means:

  • single-feature output is byte-identical (['t_lag0', 't_lag1'] before and after);
  • no existing test needed changing. My first pass did rename, and it broke three tests
    that encode the current scheme — a good signal it belongs in its own PR.

Happy to follow up with the rename if you want it.

Result

Lag  ['A_lag0', 'B_lag0', 'A_lag1', 'B_lag1'] | row0 [  1. 100.   0.   0.]
Roll ['A_roll0', 'B_roll0', 'A_roll1', 'B_roll1'] | row0 [  1. 100.   2. 200.]
Cyc  ['A_cyclic0', 'B_cyclic0', 'A_cyclic1', 'B_cyclic1']

Tests

Five added to tests/test_temporal.py, using a two-column fixture where B = 100 * A so
every column is identifiable from its value alone:

  • one per transformer asserting names against the real column contents
  • a parametrised check that default (None) names still work for single-feature input
  • a width check that len(get_feature_names_out()) == transform(X).shape[1]

Full suite: 487 passed, 9 xfailed, no existing test modified. ruff check clean on changed
files; pyright unchanged at its pre-existing 71 errors.

🤖 Generated with Claude Code

All three temporal transformers hstack one ``(n_rows, n_features)`` block per
lag / statistic / component, so their columns are block-major. They inherited
``BasePreTabTransformer.get_feature_names_out``, which walks
``zip(input_features, self._output_sizes())`` and therefore emits feature-major
names. For any input with more than one column the two disagreed:

    Lag names : ['A_lag0', 'A_lag1', 'B_lag0', 'B_lag1']
    Lag row 0 : [1., 100., 0., 0.]     # col 1 is B's lag-1, labelled A_lag1

Override ``get_feature_names_out`` in each class to iterate blocks outermost.

The existing ``lag{j}`` / ``roll{j}`` / ``cyclic{j}`` suffix scheme is kept
deliberately, so single-feature output is byte-identical and no existing test
changes. The issue also suggested naming the columns after the statistic and
after sin/cos, which is more informative but changes public output names for
users whose labels are correct today; that is left as a separate change.

Closes #15

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(temporal): output column names do not match column order

1 participant