fourier_features discarded the series it was handed - #46
Merged
Conversation
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Sy1WqyZZTXcoyFYAjVbyiu
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fourier_features(close, ...)evaluatedpl.col(close) if isinstance(close, str) else closeas 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.The basis itself is a legitimate feature - Fourier seasonality terms - so the fix makes the function say what it does rather than change what it computes.
closeis kept for callers that pass it, deprecated, and now warns that it is not read.periodis 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 it: output invariant under a reversed
closeand a rescaled feature column; one full cycle perperiodrows against a closed-form sine; and the two warnings.3572 pass, 64 skipped.
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.🤖 Generated with Claude Code
https://claude.ai/code/session_01Sy1WqyZZTXcoyFYAjVbyiu