From b65a34b6d973e9d90c167eb7ea2a972cbae2d56f Mon Sep 17 00:00:00 2001 From: Stefan Jansen Date: Tue, 8 Sep 2026 20:08:04 -0400 Subject: [PATCH] fourier_features discarded the series it was handed The function evaluated `pl.col(close) if isinstance(close, str) else close` as a bare statement and threw the result away, then returned sin and cos of the row index. No price or other data has ever entered the output, while the docstring said it captured periodic patterns in price data. A caller reading that gets a deterministic seasonal basis and believes it is a spectral summary of the series. The basis itself is a legitimate feature, so the fix is to make the function say what it does rather than to change what it computes. `close` is kept for callers that pass it, deprecated, and now warns that it is not read. Two traps stated rather than left implicit. The basis is keyed on row POSITION, not on a timestamp, so it describes the intended cycle only where rows are sorted and regularly spaced; across a weekend or a filtered panel, position and time part company. And `period` is in rows: the 390 default is one US equity session in one-minute bars and describes a 390-session cycle on daily bars, so defaulting it now warns. The existing test checked feature names and the [-1, 1] bound, both of which hold for a function that ignores its input, which is why it passed throughout. Four tests replace that: output invariant under a reversed close and a rescaled feature column, one full cycle per period rows against a closed-form sine, and the two warnings. 3572 pass. Found by the band-c agent reviewing 09_model_based_features/05_spectral_features, whose prose described the function as capturing periodic structure in the price series - false of every version of it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Sy1WqyZZTXcoyFYAjVbyiu --- .../engineer/features/ml/fourier_features.py | 57 +++++++++++++++---- tests/test_ml_features.py | 54 +++++++++++++++++- 2 files changed, 96 insertions(+), 15 deletions(-) diff --git a/src/ml4t/engineer/features/ml/fourier_features.py b/src/ml4t/engineer/features/ml/fourier_features.py index 0f63797..d8d584a 100644 --- a/src/ml4t/engineer/features/ml/fourier_features.py +++ b/src/ml4t/engineer/features/ml/fourier_features.py @@ -1,3 +1,5 @@ +import warnings + import numpy as np import polars as pl @@ -10,33 +12,50 @@ @feature( name="fourier_features", category="ml", - description="Fourier Features - spectral features for ML", + description="Fourier Features - a deterministic seasonal basis on row position", normalized=False, formula="", ta_lib_compatible=False, ) def fourier_features( - close: pl.Expr | str, + close: pl.Expr | str | None = None, n_components: int = 10, period: int | None = None, ) -> dict[str, pl.Expr]: - """Extract Fourier features for capturing periodic patterns. + """Build a deterministic seasonal basis: sin and cos of row position. + + These are the Fourier *seasonality terms* used to let a model represent a cycle of a + known length, not a spectral transform of a series. **No price or other data enters + them.** Feature ``k`` is ``sin(2 pi k t / period)`` and ``cos(2 pi k t / period)``, + where ``t`` is the row index, so the output depends only on how many rows there are + and on ``period``. + + Two consequences worth stating, because both have been misread: - Useful for capturing complex seasonal patterns in price data. + - The basis is keyed on **row position, not on a timestamp.** It describes the cycle + you asked for only where rows are sorted and regularly spaced with no gaps. Across a + weekend, a holiday, or a filtered panel, position and time part company. + - ``period`` is measured in **rows**, not in minutes. The 390 default is one US equity + session counted in one-minute bars; on daily bars it describes a 390-session cycle, + which is almost certainly not what the caller meant. Pass the period explicitly. Parameters ---------- - close : pl.Expr | str - Time series column + close : pl.Expr | str, optional + **Deprecated and unused.** Accepted so existing callers keep working. The + function never read it: the previous implementation evaluated + ``pl.col(close) if isinstance(close, str) else close`` as a bare statement and + discarded the result, so every version of this function has returned a basis + independent of the series it was handed. Passing it now warns. n_components : int, default 10 - Number of Fourier components + Number of harmonics. Component ``k`` completes ``k`` cycles per ``period`` rows. period : int, optional - Base period (if None, assumes daily = 390 minutes for US markets) + Cycle length **in rows**. Defaults to 390 with a warning; see above. Returns ------- dict[str, pl.Expr] - Dictionary of Fourier features + ``fourier_sin_k`` and ``fourier_cos_k`` for k in 1..n_components. Raises ------ @@ -50,14 +69,28 @@ def fourier_features( if period is not None: validate_window(period, min_window=1, name="period") - pl.col(close) if isinstance(close, str) else close + if close is not None: + warnings.warn( + "fourier_features() does not read `close` and never has: it returns a " + "deterministic seasonal basis on row position. Drop the argument, and if you " + "wanted spectral content of the series, this is not the function for it.", + DeprecationWarning, + stacklevel=2, + ) if period is None: - period = 390 # Trading minutes in a day + period = 390 # one US equity session in one-minute bars + warnings.warn( + "fourier_features() period defaulted to 390 rows, which is one US equity " + "session counted in one-minute bars. On any other bar size that is a cycle " + "nobody asked for - on daily bars it is 390 sessions. Pass `period` explicitly.", + UserWarning, + stacklevel=2, + ) features = {} - # Create time index (assumes sequential data) + # Row position. Regular spacing is the caller's to guarantee; see the note above. t = pl.int_range(pl.len()).cast(pl.Float64) for k in range(1, n_components + 1): diff --git a/tests/test_ml_features.py b/tests/test_ml_features.py index e5c0f13..3ddb48b 100644 --- a/tests/test_ml_features.py +++ b/tests/test_ml_features.py @@ -213,8 +213,10 @@ def test_volatility_adjusted_returns(self, sample_data): def test_fourier_features(self, sample_data): """Test Fourier feature extraction.""" - # Test with default period - fourier_default = fourier_features("close", n_components=3) + # Test with default period. `close` is deprecated and unused; the warning is the + # point of the separate test below. + with pytest.warns((DeprecationWarning, UserWarning)): + fourier_default = fourier_features("close", n_components=3) result = sample_data.with_columns( [fourier_default[key].alias(key) for key in fourier_default], @@ -231,10 +233,56 @@ def test_fourier_features(self, sample_data): assert (result[col] <= 1).all() # Test with custom period - fourier_custom = fourier_features("close", n_components=2, period=100) + with pytest.warns(DeprecationWarning): + fourier_custom = fourier_features("close", n_components=2, period=100) assert len(fourier_custom) == 4 # 2 components * 2 (sin, cos) + def test_fourier_features_do_not_depend_on_any_series(self, sample_data): + """The basis is a function of row position and period only. + + This is what the previous test could not see. It checked names and the [-1, 1] + bound, both of which hold for a basis that ignores its input - and the + implementation did ignore it, evaluating `pl.col(close) if isinstance(close, str) + else close` as a bare statement and discarding it. Naming the property directly + means the docstring and the code now have to agree. + """ + basis = fourier_features(n_components=2, period=64) + scrambled = sample_data.with_columns( + pl.col("close").reverse().alias("close"), + (pl.col("feature1") * -100.0).alias("feature1"), + ) + original = sample_data.with_columns([basis[k].alias(k) for k in basis]) + altered = scrambled.with_columns([basis[k].alias(k) for k in basis]) + for col in basis: + assert original[col].to_list() == altered[col].to_list() + + def test_fourier_features_period_is_measured_in_rows(self, sample_data): + """Component k completes exactly k cycles per `period` rows. + + Pinning the period to rows is what makes the 390 default legible as a trap: it is + one US equity session in one-minute bars, and 390 sessions on daily bars. + """ + import numpy as np + + period = 64 + basis = fourier_features(n_components=1, period=period) + got = sample_data.select(basis["fourier_sin_1"].alias("s"))["s"].to_list() + t = np.arange(len(got), dtype=float) + expected = np.sin(2 * np.pi * t / period) + assert np.allclose(got, expected) + # One full cycle later, the basis repeats. + assert got[0] == pytest.approx(got[period], abs=1e-9) + + def test_fourier_features_warns_when_the_period_is_defaulted(self): + """390 is one US equity session in minute bars and wrong at any other bar size.""" + with pytest.warns(UserWarning, match="390 rows"): + fourier_features(n_components=1) + + def test_fourier_features_warns_when_handed_a_series(self): + with pytest.warns(DeprecationWarning, match="does not read"): + fourier_features("close", n_components=1, period=64) + def test_interaction_features(self, sample_data): """Test polynomial interaction feature creation.""" # Test degree 2 interactions