Summarise every data source in the sensor statistics table - #2462
Open
Ahmad-Wahid wants to merge 3 commits into
Open
Summarise every data source in the sensor statistics table#2462Ahmad-Wahid wants to merge 3 commits into
Ahmad-Wahid wants to merge 3 commits into
Conversation
The statistics table showed one data source at a time, while the graph beside it plots every source that has data for the range. Report an "All sources" entry alongside the per-source ones, and open the table on it, so the two line up. The entry is folded together in Python from aggregates the query already returns, so it costs no extra database work. Two other routes were measured and rejected against a 1.9M-row table carrying the reordered primary key: - Grouping on timed_belief.source_id and joining data_source afterwards, on the theory that joining before the aggregate costs a probe per belief row, is 5x worse (297 buffers and 10ms, against a 17,664-buffer parallel sequential scan and 49ms). The join is what tells the planner which sources to look for, which is what lets a time-filtered query use (sensor_id, source_id, event_start) as an index range per source. The existing query shape is therefore left alone, and a comment now says why. - A GROUP BY GROUPING SETS rollup, which is free on a time-filtered query (same plan, same buffers) but blocks parallel aggregation over a sensor's whole history: 148ms becomes 419ms. That is the path the table takes whenever "Show stats for selected duration" is unchecked. Combining in the browser, where the data already is, would have been free but gives a wrong mean. "Number of values" counts NaN rows while the sum leaves them out, so weighting each source's mean by that count understates the result. The query now also selects a NaN-excluded count, which the combination divides by and which is not itself reported. This is not hypothetical: the gas sensor fixture holds a NaN, so its combined mean divides by 44 rather than 45. The entry is reported only when more than one source recorded, since with a single source it would just repeat that source's own record. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Documentation build overview
4 files changed± changelog.html± _autosummary/flexmeasures.api.v3_0.sensors.html± api/change_log.html± api/v3_0.html |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are a few objective doc/comment-style issues (grammar in published API descriptions and mid-phrase line wraps violating repo comment/docstring conventions) that should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR aligns the sensor statistics table with the sensor graph by adding a combined “All sources” option (default) and extending the /api/v3_0/sensors/<id>/stats response to include a combined entry when multiple sources exist, with NaN-aware mean computation.
Changes:
- Add an
All sourcescombined stats entry inget_sensor_stats, computed without extra DB work and with NaN-aware mean. - Update the UI source selector to default to
All sources(when present) and list it first. - Update API docs/OpenAPI spec and add dedicated unit tests covering combined stats and NaN edge cases.
File summaries
| File | Description |
|---|---|
| flexmeasures/data/services/sensors.py | Adds ALL_SOURCES_KEY and combines per-source aggregates into an All sources record, including NaN-aware mean support. |
| flexmeasures/ui/static/js/flexmeasures.js | Adds All sources handling in the stats dropdown and makes it the default selection when available. |
| flexmeasures/data/tests/test_sensor_stats.py | New unit tests for combined stats behavior and NaN edge cases. |
| flexmeasures/api/v3_0/tests/test_sensors_api.py | Extends API test to assert presence and correctness of the combined stats entry. |
| flexmeasures/api/v3_0/sensors.py | Updates endpoint description and example to document the combined All sources entry. |
| flexmeasures/ui/static/openapi-specs.json | Updates generated OpenAPI spec description and examples to include All sources. |
| documentation/changelog.rst | Adds end-user changelog entry for the UI default behavior. |
| documentation/api/change_log.rst | Adds API changelog entry describing the new All sources response key and NaN-aware mean. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: Ahmad-Wahid <ahmedwahid16101@gmail.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.
Closes #2458.
What this does
The statistics table showed one data source at a time, while the graph next to it shows all of them. This adds an All sources option to the source selector and makes it the default, so the table and the graph now show the same thing.
Picking an individual source still works exactly as before. A
?source=link (e.g. arriving from an automation) still opens on that source. Sensors with only one source are unchanged, since a combined row would just repeat that source's row.Example
One catch: the average
You can't work the combined average out in the browser, even though the numbers are all there.
Number of valuescounts rows holding NaN, butSum over valuesskips them, so combining the per-source averages gives a slightly wrong answer.The query now also fetches a NaN-free count for the combined row to divide by. It isn't shown in the table.
Real numbers from a demo sensor (4 sources, 1000 rows each, 20 of them NaN):
Close enough to look right, which is the problem. The existing gas sensor test fixture already contains a NaN, so this is not a theoretical case.
API
GET /api/v3_0/sensors/<id>/statsreturns one extra key,All sources, when a sensor has more than one source. It can't clash with a real source, as those always end in(ID: <id>). Nothing else about the response changed.Tests
New tests in
flexmeasures/data/tests/test_sensor_stats.pycover the combined row, the NaN-aware average, a source with nothing but NaN, and the single-source case. Each one was checked against a deliberately broken implementation to confirm it fails when it should.The UI change is three small edits in
flexmeasures.jsand has no automated test.documentation/changelog.rst🤖 Generated with Claude Code