Hand the forecasting model each regressor once, not once per regressor - #2560
Merged
Merged
Conversation
… per regressor Context: - detect_and_fill_missing_values copied the whole frame on every pass of its per-sensor loop, so each pass built a Darts series holding every sensor's column. Stacking those passes gave N copies of all N columns: two past regressors reached LightGBM as components a, b, a_1, b_1. - The test covering this frame shape monkeypatches the method away, so the real one never ran against more than one regressor. Change: - Narrow the frame to the sensor's own column before converting to a Darts series, keeping the missing-column case on its existing all-NaN path. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> (cherry picked from commit 4886d91) Signed-off-by: F.N. Claessen <claessen@seita.nl>
…essor Proven to fail without the fix: the model then receives four components for two regressors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: F.N. Claessen <claessen@seita.nl>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: F.N. Claessen <claessen@seita.nl>
Documentation build overview
|
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The fix is narrowly scoped, aligns with the described root cause, and includes a focused regression test plus a changelog entry.
Pull request overview
This PR fixes a bug in the forecasting pipeline where regressors could be duplicated as Darts TimeSeries components (e.g. a, b, a_1, b_1) because the full regressor dataframe was copied and converted once per regressor, causing an N×N component explosion when stacking per-sensor series. The change ensures each per-sensor pass converts only that sensor’s own column, and adds a targeted regression test plus a changelog entry.
Changes:
- Narrow
detect_and_fill_missing_valuesto copy onlyevent_startand the current sensor column before converting to a DartsTimeSeries. - Add a regression test asserting two regressors produce exactly two components (not four).
- Document the fix in the main changelog under Bugfixes.
File summaries
| File | Description |
|---|---|
| flexmeasures/data/models/forecasting/pipelines/base.py | Prevents per-regressor stacking from duplicating all regressor columns by slicing to the current sensor’s column before Darts conversion |
| flexmeasures/data/tests/test_forecasting_pipeline.py | Adds a regression test to ensure each regressor contributes exactly one component |
| documentation/changelog.rst | Adds a user-facing bugfix entry describing the duplicated-regressor training issue |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Flix6x
added a commit
that referenced
this pull request
Sep 17, 2026
…rries it on its own Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: F.N. Claessen <claessen@seita.nl>
Flix6x
added a commit
that referenced
this pull request
Sep 17, 2026
* Let a forecast target name the sources it trains on The three regressor options accept a source-filtered sensor reference, so a regressor can say which of the sources recording on a sensor it reads. The target sensor took a bare ID only, so a sensor that several sources report on was trained on whichever of them won each event, with no way to say which one holds the truth. The target now accepts the same reference. A bare ID keeps behaving as before: every source is trained on except forecasters, which are left out so that a forecaster does not learn from its own forecasts. A reference replaces that default rather than adding to it, so pass exclude-source-types yourself to keep forecasters out alongside another filter. Forecasts are still recorded on the sensor itself, never on a source-filtered view of it, so the output sensor unwraps a referenced target. The reference survives serialization into a queued job and storage on an automation. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * docs: changelog entry for the forecast target source filter Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * data/models/forecasting: extract reusable bound parsing and application Context: - Clipping and snapping are about to be applied to forecaster inputs as well as to forecast output, so the logic can no longer live inside the output path. Change: - Split parse_bounds and apply_bounds_to_values out of apply_forecast_post_processing, keeping snap-then-clip order and the [first, second) interval semantics. - Let error messages carry a label, so input bounds can name the sensor at fault. - Move _is_parseable_quantity here from the schema, where the input reference schema can reach it too without importing the pipeline schema. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * data/models/forecasting: hand the model each regressor once, not once per regressor Context: - detect_and_fill_missing_values copied the whole frame on every pass of its per-sensor loop, so each pass built a Darts series holding every sensor's column. Stacking those passes gave N copies of all N columns: two past regressors reached LightGBM as components a, b, a_1, b_1. - The test covering this frame shape monkeypatches the method away, so the real one never ran against more than one regressor. Change: - Narrow the frame to the sensor's own column before converting to a Darts series, keeping the missing-column case on its existing all-NaN path. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * data/schemas/forecasting: clean a forecaster's inputs against per-sensor bounds Context: - lower, upper and snap only shaped the forecast on its way out. A sensor with implausible readings could not be cleaned up on its way in, so training on it meant fixing the data upstream or copying it to another sensor. Change: - Let a regressor or target reference carry lower, upper and snap, alongside the source filters it already takes. A bare sensor ID keeps working unchanged. - Hold the bounds on a forecaster-specific reference schema rather than the shared one, which flex-model and flex-context also use, where they mean nothing. - Bound each input series after its gaps are filled, so an interpolated value is bounded too, reusing the snap-then-clip order and interval semantics of the output path. - Carry the bounds through the queued-job payload, omitting them when unset so a reference without cleaning serialises exactly as before. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * data/tests: cover input bounds and the one-component-per-regressor fix Context: - Forecaster inputs can now carry cleaning bounds, and filling no longer duplicates regressor columns. Neither was covered. Change: - Test that bounds clip, snap and reach values interpolated across a gap, that they read the regressor's own unit rather than the target's, and that an incompatible unit is refused by name. - Test that two regressors reach the model as two components. - Name the bounded input in quantity-conversion errors too, so a bad bound on a regressor no longer reports itself as forecast post-processing. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * data/schemas/tests: cover cleaning bounds on forecaster references Context: - Regressor and target references now take lower, upper and snap, and the shared sensor reference deliberately does not. Change: - Test that a regressor and a target load their bounds, that an unparseable bound is refused at load time, and that a reference asking for neither bounds nor filters still collapses to the plain sensor. - Test that the shared sensor reference, which flex-model and flex-context use, refuses the bound keys that the forecaster's own reference accepts. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * docs: describe cleaning a forecaster's inputs Context: - lower, upper and snap can now clean a regressor or the target before training, not only shape the forecast on its way out. Change: - Add a section covering where the bounds go, that each sensor's bounds are read in its own unit, and that input and output bounds are configured separately. - Say plainly that bounding runs after gaps are filled, including what that costs when a bad reading sits next to a gap. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * docs/changelog: note input cleaning bounds and the duplicated-regressor fix Context: - Both are user-visible: a new way to configure a forecaster, and a correction to what a forecaster with several regressors trained on. Change: - Add a New features entry for the cleaning bounds and a Bugfixes entry for the duplicated regressor data, both pointing at PR #2555. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * utils/unit_utils: read a quantity string without catching every exception Context: - Schemas need to tell a usable bound from an unusable one while loading, and the forecaster's copy of that check caught a bare Exception, against the repo's error-handling guideline. - The check has to live somewhere both schemas/sensors.py and the forecasting models can reach, and the latter already imports the former. Change: - Add is_parseable_quantity here, catching QUANTITY_PARSE_ERRORS: pint's own errors plus the tokenizer and arithmetic errors its expression parser surfaces for input like '', '[[[' or '1/0', which PintError alone misses. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * utils/unit_utils: stop reassigning a str parameter to str | None Context: - Adding a typing import to this module brought it into mypy's file list, which is built by grepping for "from typing import", and that surfaced a latent error in split_into_magnitude_and_unit. Change: - Return the formatted magnitude through its own local instead of writing it back over the str parameter. Behaviour is unchanged, as its doctests show. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * data/schemas: let any sensor reference carry cleaning bounds Context: - Review asked for the bounds to sit on the shared sensor reference rather than a forecaster-specific one, since they are at least as useful for the data the scheduler reads. `default` already sits there without applying everywhere. Change: - Move lower, upper and snap onto SensorReferenceSchema and SensorReference, and return a reference whenever either bound keys or source filters are given. - Drop the forecaster-specific reference schema, field and dataclass, which the forecasting pipeline no longer needs. - Say on each field where the bounds are honoured so far, the way `default` documents its own gap: forecaster inputs act on them, flex-model and flex-context accept and ignore them until the scheduler follow-up. - This also corrects the published OpenAPI, which advertised a plain SensorReference for a field that accepted more. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * data/tests: follow the cleaning bounds onto the shared sensor reference Context: - The bounds moved off the forecaster-specific reference, so the test that pinned them off the shared schema now asserted the opposite of the design. Change: - Point the pipeline and schema tests at SensorReference. - Replace the isolation test with its inverse: the shared reference takes the bounds, and still refuses one that cannot be read as a quantity. - Add a test that a reference setting no bounds serialises as it did before, while a meaningful zero bound survives. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * data/schemas/forecasting: say that references take bounds, and flag ensure-positive Context: - Review found the regressor and target descriptions still describing the dict form as source filtering only, which misleads anyone writing config, and asked for ensure-positive to be marked as intended for deprecation. Change: - Say on the regressor and target fields that a reference can also carry bounds, and spell out on the target that input bounds clean what the model learns from while the forecaster's own bounds shape what it writes back out. - Mark ensure-positive as meant for deprecation, in its description and beside the line that applies it, pointing at the explicit lower bound instead. - Catch pint's parse errors rather than every exception when reading a bound. - Re-wrap the snap docstring to break only after punctuation. - Say in the payload docstring that bounds are serialised alongside filters. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * docs: note that the bounds live on any sensor reference Context: - The bounds moved onto the shared sensor reference, so flex-model and flex-context references accept them, while only forecaster inputs act on them. Change: - Say so, rather than leaving a reader to find out that a bound set on a scheduling reference is quietly ignored. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * utils/bound_utils: share bound parsing and validation between schemas and generators Bound parsing and application move out of the forecasting utils into their own module, so that sensor references and schedulers can use them without importing forecasting code. The parseability check that the forecaster config and the sensor reference schema both repeated is now one helper. The quantity-parsing helpers in unit_utils now sit below the registry setup instead of splitting it in two. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: F.N. Claessen <claessen@seita.nl> * data/schemas/sensors: refuse bounds a sensor reference cannot apply when it is loaded The referenced sensor is already loaded when its bounds are validated, so an incompatible unit, a snap target outside its interval and a lower bound above the upper bound are now refused up front, instead of surfacing inside the queued job once its data has been read. The three schema tests that bounded an MWh sensor in kW relied on that gap, and now use the MW dummy sensor. A reference also parses its bounds once, and applies them itself, rather than having each read parse them again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: F.N. Claessen <claessen@seita.nl> * data/schemas/sensors: refuse bounds on flex-model and flex-context references until scheduling applies them The bounds live on the shared sensor reference, but only forecaster inputs act on them for now. A flex-model or flex-context reference used to accept them and silently drop them, without even checking them, since those references are deserialized by their own field rather than by the shared schema. Refusing them keeps a release from shipping bounds that look configured but do nothing. Scheduling will apply them in a follow-up, which lifts this refusal. The shared bound validation also checks the shape of each snap entry itself now, so it reports a malformed interval instead of failing on it, whichever field feeds it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: F.N. Claessen <claessen@seita.nl> * data/schemas/sensors: say that references carry cleaning bounds, and break a docstring after punctuation Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: F.N. Claessen <claessen@seita.nl> * docs/changelog: leave the duplicated-regressor fix to #2560, which carries it on its own Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: F.N. Claessen <claessen@seita.nl> * data/schemas/sensors: only refuse bounds a scheduling reference actually sets, not an explicit null or empty snap Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: F.N. Claessen <claessen@seita.nl> * utils/unit_utils: let split_into_magnitude_and_unit swallow only quantity parse errors Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: F.N. Claessen <claessen@seita.nl> * data/schemas/forecasting: say that ensure-positive defaults to false, as it does Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: F.N. Claessen <claessen@seita.nl> * data/schemas/sensors: keep a forecaster reference with only unset bounds a plain sensor An explicit null bound or empty snap no longer turns a regressor or target into a reference, so its queued-job payload stays a bare sensor ID. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: F.N. Claessen <claessen@seita.nl> * utils: refuse a boolean as a bound, and say that bounding leaves NaN values alone A JSON true or false is a numbers.Real to Python, so it passed as a bound of 1 or 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: F.N. Claessen <claessen@seita.nl> * data/schemas/sensors: read an explicit null snap as unset, as null lower and upper bounds already are The shared reference schema refused it, while the flex-config field accepted it, so the same reference loaded in one place and failed in another. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: F.N. Claessen <claessen@seita.nl> --------- Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> Signed-off-by: F.N. Claessen <claessen@seita.nl> Co-authored-by: F.N. Claessen <claessen@seita.nl> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
Description
Closes #2559.
detect_and_fill_missing_valuescopied the whole frame on every pass of its per-sensor loop, so each pass built a Darts series holding every sensor's column.Stacking those passes gave N copies of all N columns: two past regressors reached LightGBM as components
a, b, a_1, b_1.This narrows the frame to the sensor's own column before it is converted to a Darts series.
The fix was written by @BelhsanHmida as part of #2555, and is split out here (his commit, cherry-picked) so that it can land, and be backported, independently of that feature.
documentation/changelog.rstLook & Feel
No visible change, other than forecasts from a forecaster with two or more regressors of the same kind changing, since the model no longer sees each of those regressors several times over.
How to test
flexmeasures/data/tests/test_forecasting_pipeline.py::test_filling_gives_each_regressor_exactly_one_componentReverting the fix makes it fail with four components instead of two.
The existing pipeline tests with several regressors did not catch this, because they monkeypatch this method away.
Further Improvements
This bug is in released versions, so it is a candidate for a v1.0.x backport.
Related Items
Sign-off
🤖 Generated with Claude Code