[BUG] Normalize decoder target when using EncoderNormalizer in EncoderDecoderDataModule - #2361
Open
Zuhef wants to merge 1 commit into
Open
[BUG] Normalize decoder target when using EncoderNormalizer in EncoderDecoderDataModule#2361Zuhef wants to merge 1 commit into
Zuhef wants to merge 1 commit into
Conversation
`EncoderDecoderTimeSeriesDataModule.__getitem__` normalized `target_past` through the per-sequence normalizer but returned `y` straight from the still-raw cached target. `_normalize_target` deliberately skips global normalization when the normalizer is per-sequence, so with an `EncoderNormalizer` the decoder target was never normalized at all and sat in a different space than the encoder input. Add `ScalerAdapter.transform_sequence`, a companion to `fit_transform_sequence` that transforms only per-sequence sub-normalizers using their already-fitted state, and apply it to `y`. Transforming rather than re-fitting keeps `y` on the encoder window's parameters and avoids leaking future values into the scaling. Fixes sktime#2360
Zuhef
requested review from
benHeid,
fkiraly,
jdb78 and
phoeenniixx
as code owners
July 29, 2026 21:14
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2361 +/- ##
=======================================
Coverage ? 87.34%
=======================================
Files ? 171
Lines ? 10094
Branches ? 0
=======================================
Hits ? 8817
Misses ? 1277
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
LLM generated content, by claude-opus-5
(Disclosed per the note in this repository's pull request template. The change was
reviewed, executed and verified locally by a human-directed workflow; all test output
quoted below was produced on this branch.)
Reference Issues/PRs
Fixes #2360.
Touches the same code path as #2302, which introduced the per-sequence scaler handling.
What does this implement/fix? Explain your changes.
With an
EncoderNormalizer,EncoderDecoderTimeSeriesDataModulereturned a decodertarget
ythat was never normalized, soysat in a different space thanx["target_past"].The reason it is skipped entirely rather than merely inconsistent:
_normalize_targetapplies the global transform only when the normalizer is not per-sequence.
So for an
EncoderNormalizerthe cacheddata["target"]stays raw for the wholeseries, and normalization is deferred to
__getitem__. There,target_pastisfitted-and-transformed on the encoder window, but
ywas taken straight from thatstill-raw cache. Net effect: the encoder input is standardized while the training
target is not.
Reproduction on
main(deterministic series with a strong trend, encoder length 20,prediction length 5):
The fix adds
ScalerAdapter.transform_sequence, a companion to the existingfit_transform_sequence, and applies it toy:fit_transform_sequencejust fitted on theencoder window, rather than re-fitting on the decoder window. Re-fitting would leak
future values into the scaling and would put
yin a different space thantarget_past— arguably worse than the original bug;fit_transform_sequencecolumn-by-column forMultiNormalizer, sonon-per-sequence sub-normalizers are passed through untouched and never
double-normalized.
After the fix the same window gives
y[0]: mean=+2.1108 std=0.2769, which is thearithmetically expected result: the encoder window spans 500→880 (mean ≈ 691,
std ≈ 118.7) and the decoder window sits at ≈ 941, so
(941 − 691) / 118.7 ≈ 2.11.stdstays at 0.277 rather than collapsing to 1.0,confirming the encoder parameters were reused instead of re-fitted.
What should a reviewer concentrate their feedback on?
y" to mean "apply the encoderwindow's fitted parameters", not "fit on the decoder window". Please confirm that
matches your intent, since it is the one genuinely semantic decision here.
transform_sequencelives onScalerAdapternext to
fit_transform_sequence. Happy to inline it in the data module instead ifyou would rather not widen the adapter's public surface.
MultiNormalizerthatmixes per-sequence and global normalizers,
ScalerAdapter.fit_per_sequenceisany(...), so_normalize_targetskips the global transform for every column —meaning the non-per-sequence targets are currently never normalized at all. That is
independent of this issue, so I left it alone and only kept the decoder side
symmetric with the encoder side. Happy to open a separate issue if that is a real
bug rather than intended.
Did you add any tests for the change?
Yes, three in
tests/test_data/test_data_module.py:test_encoder_normalizer_normalizes_decoder_targetyequals the raw decoder target transformed with the encoder window's fitted parameters. Fails onmain.test_encoder_normalizer_normalizes_decoder_target_multivariateMultiNormalizer: per-sequence column transformed, non-per-sequence column left untouched. Fails onmain.test_encoder_normalizer_decoder_target_does_not_refitmaintoo — it constrains the fix rather than reproducing the bug.Expectations are derived from the data module's own
target_originalcache for thesame window, so they do not depend on two data modules happening to select the same
series.
Verification run locally on Windows / Python 3.13 / torch 2.13.0+cpu:
The new tests were run three times consecutively to check they are not order- or
seed-dependent.
Any other comments?
The diff is 181 added lines and no deletions or modifications to existing logic. I did
not add a
CHANGELOG.mdentry, following #2302 / #2300 / #2256 which leave it tomaintainers at release time — glad to add one if you prefer.
PR checklist
pre-commit install.To run hooks independent of commit, execute
pre-commit run --all-files