Count an event once in a KPI, when several sources report it - #2472
Merged
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>
Documentation build overview
36 files changed ·
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are determinism and coverage gaps (tie-breaking behavior and a test/doc mismatch around “latest source version”) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adjusts KPI calculation on the asset page so that when multiple data sources report the same event, the KPI reduces one deterministic value per event (instead of summing multiple “claims” into a misleading total). This aligns KPI semantics with “one value per event” while keeping the chart behavior (still showing all sources) explicitly documented.
Changes:
- Update the KPI query to request
one_deterministic_belief_per_event=True, so the KPI reduces a single value per event. - Add a regression test covering the “two sources, same event” scenario.
- Document the new KPI-vs-chart behavior and add a bugfix changelog entry.
File summaries
| File | Description |
|---|---|
flexmeasures/api/v3_0/assets.py |
Changes KPI belief search to select a single deterministic belief per event |
flexmeasures/api/v3_0/tests/test_assets_api.py |
Adds a regression test for multi-source reporting of the same event |
documentation/views/asset-data.rst |
Documents that KPIs reduce one value per event and may differ from chart point counts |
documentation/changelog.rst |
Adds a bugfix entry describing the corrected KPI behavior |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A newly added docstring violates the repository’s “break lines only after punctuation” convention, and should be reformatted before approval.
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
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>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The change is small and well-targeted, with clear documentation updates and new tests covering the previously incorrect KPI behavior and its selection rules.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Flix6x
commented
Sep 4, 2026
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>
3 tasks
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>
Flix6x
added a commit
that referenced
this pull request
Sep 8, 2026
#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>
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
Two data sources reporting the same event are two claims about it, not two contributions to it —
but a KPI added them up. The total then showed a number no source ever reported, and that no point on
the chart beside it showed either.
One sensor, one day, reported by two sources:
A
sumKPI over that day reported 180.0. It now reports 80.0, the more recent belief aboutthe event.
This is direction 1 from #2471, where the alternatives are written up.
Why
most_recent_beliefs_onlywas not enoughThat filter is per source, so it leaves one row per event per source. The default
use_latest_version_per_event=Truedoes collapse sources that share a(name, type, model), butonly within one belief time, so two sources believing the same event at different moments both
survive into the values the KPI reduces.
The KPI query now also asks for
one_deterministic_belief_per_event=True, which picks one value perevent: latest source version first, most recent belief within that second.
What changes for users
A KPI over a sensor that only ever has one source per event is unaffected — which is the common case,
and every existing KPI test but the new one covers it.
Where a sensor does carry several sources for one event, the KPI now reports one of their values
instead of their sum. The chart still draws every source, so it can show more points than the KPI
counted. The KPI documentation says so now, since the two no longer agree point for point in that
case — the previous behaviour agreed with the chart by summing what it drew, which is exactly what
produced the misleading total.
documentation/changelog.rstHow to test
test_kpi_counts_an_event_once_when_two_sources_report_itis the example above. It fails onmainwith
assert 180.0 == 80.0 ± 8.0e-05.test_kpi_reports_what_the_chart_drawsstill passes unchanged: its two sources report differentevents, so summing across them was always the right answer, and still is.
Related items
Closes #2471. Found while reviewing #2464, which makes a scheduled sensor able to carry schedules from
several sources and so widens the window in which this bites, but the behaviour predates it and
reproduces on
main.Sign-off
🤖 Generated with Claude Code
https://claude.ai/code/session_01Lp1bUhWjEQtyDbnvRZQgQs