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