Fix Fourier periods under scaling

[drbenvincent]

Closes #495

[drbenvincent]

Review verdict: APPROVE. The implementation preserves Fourier inputs in their declared units for both model fitting and simulation, including columns also used as plain regressors. The added public-model regression compares the materialized basis against a NumPy oracle computed from raw weeks. I found no correctness, edge-case, contract, or test issues. GitHub does not allow the PR author to submit an approval, so this is posted as a comment review.

[drbenvincent]

Review verdict: APPROVE. The round-2 change moves factor filtering behind ScalingFactors.without_columns(), preserving the scaling abstraction boundary. The model and simulation paths both exclude Fourier inputs from fitted target/channel scales, and reused fitted factors are filtered consistently. The public-model regression covers the stated shared raw-regressor case against an independent NumPy Fourier oracle. I found no remaining correctness, contract, edge-case, or test issues. GitHub does not allow the PR author to submit an approval, so this is posted as a comment review.

[daimon-pymclabs]

Adversarial review: request changes

The fix does resolve #495: Fourier inputs keep their declared units, and tests/test_basis.py plus tests/test_scaling*.py pass locally (171 passed). The problem is that the fix drops scaling the user asked for without any warning in three cases, all reproduced against 2c5e3ef.

1. A mediator used as a Fourier input loses its target scaling

x = np.arange(40.)
d = pd.DataFrame({"x": x, "m": 2 * x + 1, "y": rng.normal(size=40)})
m = pathmc.model(
    "m ~ x\ny ~ fourier(m, n=1, period=52)",
    data=d,
    scaling=pathmc.Scaling(target={"method": "max"}, channel={"method": "max"}),
)
m.fitted_scaling.columns   # ('x', 'y')
m._data["m"].max()         # 79.0

The PR subtracts fourier_inputs from target_columns as well as channel_columns, so m, which is an outcome of its own regression, gets fitted on its raw scale. Its likelihood and priors change and nothing tells the user. Either keep target scaling for m and feed the Fourier basis an unscaled copy, or raise or warn.

2. A column used as both a plain regressor and a Fourier input goes unscaled in both roles

m = pathmc.model(
    "y ~ week + spend + fourier(week, n=2, period=52)",
    data=d,  # week = 0..103
    scaling=pathmc.Scaling(channel={"method": "max"}),
)
m.fitted_scaling.columns   # ('spend',)

The week beta prior now applies to raw values from 0 to 103, while every other channel's prior applies to values from 0 to 1. The Scaling docstring mentions this, but users get no warning and the prior mismatch is hard to spot. A safer design is to scale the plain-term column as requested and pass the raw values only to the basis.

3. An explicit fixed scaling on a Fourier input is dropped silently

pathmc.model("y ~ fourier(week, n=1, period=52)", data=d,
             scaling=pathmc.Scaling(channel={"method": "fixed", "value": 52.0})).fitted_scaling.columns
# ()

When the user explicitly configures scaling for this column, the code should raise or at least warn instead of ignoring it.

Design point: hardcoded basis name

t.basis.name == "fourier" is duplicated in model() and simulate(). Because register_basis() supports custom bases, a periodic custom basis gets no protection. A BasisCapabilities flag such as requires_raw_input_units would be a better hook, and one helper for collecting the inputs would remove the duplication.

Test gaps

The new tests cover only the shared plain-regressor case and the reused-factors case. Cases 1 and 3 have no tests, and nothing exercises the simulate() path.

🤖 Generated with Claude Code

[drbenvincent]

Addressed in commit 7e819a6:

1. Fourier-capable mediator inputs remain in their declared units, and requested target scaling now emits a warning rather than being silently dropped.
2. A column used by both a plain regressor and a raw-unit basis remains unscaled in both roles; the warning states this explicitly so the prior-scale tradeoff is visible.
3. Explicit fixed scaling and reused fitted factors now produce the same warning when excluded for a basis input.
4. The policy is declared through BasisCapabilities.requires_raw_input_units, so registered custom bases can opt in without hardcoded name checks.
5. Added regressions for target scaling, fixed scaling, reused factors, and simulate().

Focused basis/scaling/contract tests pass (174 tests), and make lint passes.

[drbenvincent]

Round 3 review verdict: APPROVE. The round-2 findings are addressed: raw-unit basis inputs are collected through BasisCapabilities.requires_raw_input_units; skipped target and channel scaling emits a warning that also explains the plain-regressor tradeoff; fixed and reused factors are covered; and simulate() now follows the same policy. The added regressions exercise the mediator target case and compare Fourier values to a NumPy oracle based on declared input units. I found no remaining correctness, contract, or test issues. GitHub does not allow the PR author to submit an approval, so this is posted as a comment review.

[daimon-pymclabs]

Re-review of 7e819a6: approve with comments

Everything from my first review is addressed. The new requires_raw_input_units capability flag replaces the hardcoded basis name, scaling that gets skipped now raises a warning, and there are new tests for the mediator, explicit fixed and simulate() cases. The basis, scaling and HSGP tests pass locally (248 passed). Neither point below blocks the merge.

1. The warning fires on the most common MMM setup

pathmc.model(
    "y ~ spend + fourier(week, n=2, period=52)",
    data=d,
    scaling=pathmc.Scaling(channel={"method": "max"}),
)
# UserWarning: Scaling was requested for basis input column(s) ['week'] ...
#   Scaling is also skipped for plain-regressor uses of the same columns.

A blanket channel scaling combined with a seasonal Fourier term is the standard setup, and it now warns on every fit even though the behavior is correct. The message also mentions plain-regressor uses when none exist. If people learn to ignore or suppress the warning, it stops protecting the cases that matter. Suggestion: warn only when the column also has a scaled role (a plain regressor or a target), or when the user supplied per-column ScalingFactors or fixed scaling for it. Otherwise, tailor the message to the roles that actually lost scaling.

2. Adding a Fourier term changes the input scale of an HSGP term on the same column

pathmc.model("y ~ hsgp(week, m=10, c=1.5)", data=d, scaling=pathmc.Scaling(channel={"method": "max"})).fitted_scaling.columns
# ('week',)  -> HSGP sees scaled week
pathmc.model("y ~ spend + hsgp(week, m=10, c=1.5) + fourier(week, n=1, period=52)", data=d, scaling=pathmc.Scaling(channel={"method": "max"})).fitted_scaling.columns
# ('spend',) -> HSGP now sees raw week (0..103)

I haven't checked whether HSGP's default lengthscale priors assume a scaled input. If they do, adding the Fourier term quietly changes the smooth. This combination is rare, so a line in the warning or a test that pins down the intended behavior would be enough.

Not blocking

The mediator and shared-column cases still leave those columns unscaled, now with a warning. That is a defensible design choice.

🤖 Generated with Claude Code