Settle which source wins an event with one rule, in two steps - #2494
Conversation
Two sources reporting one event are two claims about it, not two contributions to it, so adding them up produced a number no source ever reported, and that no point on the chart showed. A sensor with a forecast later corrected by an upload, or with two forecasters configured differently, totalled both. The KPI query now asks for one deterministic belief per event, which prefers the latest source version, and the most recent belief within it. `most_recent_beliefs_only` alone did not do this: it is per source, and `use_latest_version_per_event` only collapses sources sharing a name, type and model, and only within one belief time. The chart beside the KPI still draws every source, so it can show more points than the KPI counted, which the KPI documentation now says. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lp1bUhWjEQtyDbnvRZQgQs Signed-off-by: F.N. Claessen <claessen@seita.nl>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lp1bUhWjEQtyDbnvRZQgQs Signed-off-by: F.N. Claessen <claessen@seita.nl>
The regression test only varied the belief time, while the docstring and the documentation also claimed a preference for the latest source version, which the fixture's unversioned sources could not show. A second test now gives one reporter two versions and has the newer one speak first, so the KPI answers with the newer version's value despite the older belief time. Ties beyond that are left alone, and `_select_latest_version_and_belief_per_event` now says why: beliefs which tie on version and belief time keep the order they came in, which is how a caller expresses its own precedence by the order it passes its sources. `test_source_transition` documents and relies on that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LtMZ49GH6LaiY5MdgtfNe2 Signed-off-by: F.N. Claessen <claessen@seita.nl>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LtMZ49GH6LaiY5MdgtfNe2 Signed-off-by: F.N. Claessen <claessen@seita.nl>
Only the changelog conflicted, where both sides added entries to the v1.1.0 bugfix list, so both sets are kept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WS6V8nZyRzMxsvqGnNpUTk Signed-off-by: F.N. Claessen <claessen@seita.nl>
A version number orders the releases of one source and says nothing about a different one, but the choice between sources ranked every version in the frame together, so a forecaster at v9 outranked a scheduler at v1 on the strength of the number alone, against a fresher belief and against a caller that had asked for the scheduler. Sources are now grouped into families sharing a name, type and model, and versions are only compared inside one. Between families, the order in which a caller passed its sources decides. That is what the `AggregatorReporter` has documented since #819 — "the first source defined in the sources array is prioritized" — and what it never did: the winner was whichever source name sorted first, so `test_source_transition` passed because "source1" sorts before "source2", and reversing the list changed nothing. It does now, which that test also checks. What neither settles falls to the most recent belief, and then to the highest source id, which does not move when a source is renamed. The probabilistic path used to repeat the same ranking in pandas, with the same cross-family bug, so it now makes its beliefs deterministic and hands them to the one implementation. `keep_latest_version` is skipped when one belief per event is asked for, since that path settles versions itself, family by family, and it would otherwise drop a fresher belief before the choice was made. Closes #2476. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WS6V8nZyRzMxsvqGnNpUTk Signed-off-by: F.N. Claessen <claessen@seita.nl>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WS6V8nZyRzMxsvqGnNpUTk Signed-off-by: F.N. Claessen <claessen@seita.nl>
A blanket replacement of the placeholder PR number also rewrote the links of two unrelated entries that legitimately cite PR #2483. Only this branch's own entry points at #2494. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WS6V8nZyRzMxsvqGnNpUTk Signed-off-by: F.N. Claessen <claessen@seita.nl>
There was a problem hiding this comment.
🟡 Changes recommended
A newly added docstring in time_series.py violates the repo’s line-break-after-punctuation convention and should be reflowed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR consolidates “which data source wins an event” into a single, deterministic selection rule used consistently across deterministic and probabilistic belief-selection paths, aligning implementation with the documented AggregatorReporter behavior and fixing cross-family version comparisons.
Changes:
- Implement a two-step precedence algorithm in
_select_latest_version_and_belief_per_event(within-source-family: version → recency → id; between families: caller order → recency → id), and route probabilistic selection through the same implementation after median-reduction. - Expand/strengthen test coverage to pin the rule’s behavior (including caller source order and cross-family version non-comparability), plus update
AggregatorReportertest to assert reversed precedence. - Add a changelog entry describing the user-visible behavior change.
File summaries
| File | Description |
|---|---|
| flexmeasures/data/tests/test_search_postprocessing.py | Adds deterministic reference implementation + focused tests asserting the new precedence rule and equivalence with the vectorized selector. |
| flexmeasures/data/models/time_series.py | Introduces the unified precedence implementation and routes selection paths through it; updates parameter docs accordingly. |
| flexmeasures/data/models/reporting/tests/test_aggregator.py | Strengthens test_source_transition to assert caller-provided source order affects the overlap event winner. |
| documentation/changelog.rst | Documents the bugfix and new deterministic precedence rule for end users. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WS6V8nZyRzMxsvqGnNpUTk Signed-off-by: F.N. Claessen <claessen@seita.nl>
Documentation build overview
6 files changed ·
|
There was a problem hiding this comment.
🟡 Changes recommended
Source precedence can become unintentionally dependent on DB-return ordering when source is provided as a single name that expands to multiple DataSources, which can contradict the intended tie-break rules.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
A source name is not unique, and the query that resolves one has no ordering, so a caller naming a single source could hand the choice several of them in whatever order the database returned. Read as a precedence, that would have let the database decide, which is the opposite of the point. The sources of one entry now share a rank, and are told apart by belief time and then id, like any other tie. `parse_source_arg_per_entry` keeps that grouping, and `parse_source_arg` flattens it, so the older function behaves exactly as before. Also shorten the changelog entry to what a reader of the changelog needs, and leave the rule itself to the docstring and the pull request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WS6V8nZyRzMxsvqGnNpUTk Signed-off-by: F.N. Claessen <claessen@seita.nl>
There was a problem hiding this comment.
🟡 Changes recommended
Source-preference ranking can be incorrect when the caller includes unknown source ids/names, because empty preference groups can shift positions and cause explicitly listed sources to tie with (or fall behind) unlisted sources.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
…rest A caller can name a source this database does not know, and the entry then matches nothing. Those entries still count when ranking the ones that did match, which is right, but the rank standing for "nobody named this" was the number of sources found rather than the number of entries given. Name two unknown sources and then a real one, and the real one ranked behind the sources nobody had named at all — the opposite of what naming it meant. The rank for the unnamed now clears every entry, matched or not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WS6V8nZyRzMxsvqGnNpUTk Signed-off-by: F.N. Claessen <claessen@seita.nl>
There was a problem hiding this comment.
🟢 Approval recommended
The behavioral changes are well-covered by targeted tests and align the implementation with the stated precedence rules; remaining feedback is limited to minor typing/maintainability nits.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
flexmeasures/data/models/time_series.py:929
- The
preferred_sourcesparameter is used as a list of entries (each entry being either a singleDataSourceor a group likelist[DataSource]/tuple[DataSource, ...]), but the current annotation (list | None) is too loose and makes it easy to call this helper with the wrong element types (which would fail at runtime when accessing.id). Tighten the type to match the expected structure so callers and type-checking can catch mistakes earlier.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
The parameter takes either a source or a group of them per entry, which `list | None` did not say, so a caller could pass the wrong thing and only find out when the ranking reached for an id. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WS6V8nZyRzMxsvqGnNpUTk Signed-off-by: F.N. Claessen <claessen@seita.nl>
There was a problem hiding this comment.
🔵 Needs a closer look
The newly added “naive” reference implementation in test_search_postprocessing.py still doesn’t mirror the production selector’s per-entry (grouped) preference semantics, weakening it as a long-term oracle for equivalence tests.
Review details
Suppressed comments (1)
flexmeasures/data/tests/test_search_postprocessing.py:52
naive_select_latest_version_and_belief_per_eventstill treatspreferred_sourcesas a flat list and useslen(positions)as the “unlisted” sentinel, but the production selector now treats each entry as one preference (possibly a group) and uses the number of entries (including empty/unknown ones) as the sentinel. This makes the naive implementation a less faithful oracle for grouped preferences and can let future equivalence tests drift from production semantics.
positions: dict = {}
for position, source in enumerate(preferred_sources or []):
positions.setdefault(source.id, position)
unlisted = len(positions)
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
The plainly written reference still read each entry as one source and still took the rank for the unnamed from the sources it found, so it encoded both of the mistakes the selector has since stopped making. An oracle that agrees with the old rule cannot catch a return to it: the equivalence test passed only because its trials never named a group, nor anything this database does not know. The reference now follows the same two rules, and the trials include an entry naming two sources and an entry naming nothing. Reintroducing either mistake in the selector now fails the equivalence test, which is what it is there for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WS6V8nZyRzMxsvqGnNpUTk Signed-off-by: F.N. Claessen <claessen@seita.nl>
There was a problem hiding this comment.
🟡 Changes recommended
The new preferred_sources type annotations allow nested generic Sequences, but the implementation only treats list/tuple as grouped entries, creating a mismatch that can lead to runtime errors unless the typing or the grouping check is aligned.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
flexmeasures/data/tests/test_search_postprocessing.py:54
- This reference implementation types
preferred_sourcesas allowing nestedSequence[DataSource], but only treats(list, tuple)as a grouped entry. Align the runtime check with the annotation (or narrow the annotation), otherwise passing a non-list/tuple sequence would be treated as a single element and break atsource.id.
group = entry if isinstance(entry, (list, tuple)) else [entry]
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
The annotation says an entry is a source or any sequence of them, while the check asked whether it was a list or a tuple, so a sequence of another kind would have been read as one source and the ranking would have reached for an id it does not have. Asking whether the entry *is* a source cannot fall behind what the annotation allows. The reference implementation had the same check, and gets the same treatment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WS6V8nZyRzMxsvqGnNpUTk Signed-off-by: F.N. Claessen <claessen@seita.nl>
There was a problem hiding this comment.
🟡 Changes recommended
A small docstring mismatch remains in TimedBelief.search where the source parameter description doesn’t explicitly state that list order affects precedence in one_deterministic_belief_per_event mode.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
… is asked for `Sensor.search_beliefs` said so, while `TimedBelief.search`, which takes the flag, and `Sensor.latest_state`, which sets it, still described the argument as a plain list. A caller reading either would have assumed the order was ignored, as it used to be. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WS6V8nZyRzMxsvqGnNpUTk Signed-off-by: F.N. Claessen <claessen@seita.nl>
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core belief-selection semantics in a highly cross-cutting query path, so a final human review is warranted to validate the precedence rule against domain expectations and potential downstream effects.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
No conflicts: main's two commits since the last merge only edit documentation, and its changelog corrections leave this branch's own entry where it belongs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WS6V8nZyRzMxsvqGnNpUTk Signed-off-by: F.N. Claessen <claessen@seita.nl>
Brings this branch up to date with its base, and so with main, by merging rather than rebasing, since the branch has been reviewed and pushed. No conflicts: what arrived is documentation, and both branches' changelog entries survive once each. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WS6V8nZyRzMxsvqGnNpUTk Signed-off-by: F.N. Claessen <claessen@seita.nl>
#2472 was squash-merged, so main now holds its content as one commit with no history in common with the commits this branch carries, and every overlapping hunk conflicts between two spellings of the same change. Merging with `-X ours` settles those by construction: where the two sides overlap they say the same thing, so this branch's side is right, and everything else from main comes in normally. This branch's own content is unchanged by the merge, at the same 5 files and +399/-103 it had against the old stack base. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WS6V8nZyRzMxsvqGnNpUTk Signed-off-by: F.N. Claessen <claessen@seita.nl>
|
Restacked onto main after #2472 was squash-merged, without a rebase or a force-push. A squash merge leaves main holding #2472's content as a single commit with no history in common with the commits this branch carries, so every overlapping hunk conflicts between two spellings of the same change. Merging main with Checked by measuring this branch's own content before and after: 5 files, +399/-103 against the old stack base, and the same 5 files and +399/-103 against main now. Nothing from #2472 was duplicated and nothing here was dropped. 1574 data and API tests pass. The base is now main, and the PR is mergeable again. |
Description
Closes #2476, which found that three different rules decided which data source wins an event, and that
the one written down was not implemented.
A version only orders one source's releases. The choice between sources ranked every version in
the frame together, so a forecaster at v9 beat a scheduler at v1 on the strength of the number alone —
against a fresher belief, and against a caller that had asked for the scheduler:
Sources are now grouped into families sharing a name, type and model — the same grouping
keep_latest_versionalready used — and versions are only compared inside one.Between families, the caller's order decides. This is what the
AggregatorReporterhas documentedsince #819, "the first source defined in the sources array is prioritized", and what it never did.
test_source_transitionpassed becausesource1sorts alphabetically beforesource2; reversing thelist changed nothing. It does now, and that test says so.
What neither settles falls to the most recent belief, and then to the highest source id, which
does not move when a source is renamed.
So the whole rule, in one place:
Also
The probabilistic path repeated the same ranking in pandas, carrying the same cross-family bug. It now
makes its beliefs deterministic and hands them to the one implementation, so there is a single rule to
read and to change.
keep_latest_versionis skipped when one belief per event is asked for. That path settles versionsitself, family by family, and running it first would drop a fresher belief before the choice was made.
How to test
pytest flexmeasures/data/tests/test_search_postprocessing.py \ flexmeasures/data/models/reporting/tests/test_aggregator.pytest_search_postprocessing.pypins each step of the chain in its own test, and cross-checks thevectorised choice against a plainly written one over random frames spanning two families, with and
without a caller's preference. Each test was confirmed to fail with the rule it covers disabled:
test_source_transitionnow also asserts the reverse ordering, so the sentence in its docstring iscovered rather than merely stated.
Full suite green: 1863 passed.
Base
This is stacked on #2472, which touches the same file and documents the tie behaviour this PR changes.
It is not stacked on #2293, despite the shared subject matter: that branch touches none of these files,
and stacking there would make a core fix wait on a large feature PR.
documentation/changelog.rstSign-off
🤖 Generated with Claude Code
https://claude.ai/code/session_01WS6V8nZyRzMxsvqGnNpUTk