diff --git a/documentation/changelog.rst b/documentation/changelog.rst index 3940b4edac..501d43c73b 100644 --- a/documentation/changelog.rst +++ b/documentation/changelog.rst @@ -30,6 +30,7 @@ Infrastructure / Support Bugfixes ----------- +* A KPI on the asset page counted an event once per data source that reported it, so a total could come out higher than any source reported; it now reduces one value per event, from the latest source version and the most recent belief within it [see `PR #2472 `_] * Sensor data ingestion now preserves ``null`` gaps when converting posted values to the sensor's unit, instead of failing the request [see `PR #2461 `_] * KPIs on the asset page counted one day more than the selected time range [see `PR #2434 `_] * KPIs on the asset page now total the values the chart beside them draws, counting each event under the day it starts in: a sensor reported by several sources counted only one of them, and a revised value was counted on top of the value it revised [see `PR #2434 `_] diff --git a/documentation/views/asset-data.rst b/documentation/views/asset-data.rst index 6ccf0f55ae..7dbba1596a 100644 --- a/documentation/views/asset-data.rst +++ b/documentation/views/asset-data.rst @@ -131,6 +131,10 @@ Currently, this supports only a daily resolution (which fits the date picker on So you will need a sensor with daily resolution (probably generated with FlexMeasures' reporting tooling). From this data, you can display summed totals, means, max or min values (the image above shows two KPIs with totals). +The function is applied to one value per event. +Where several data sources reported the same event, the value is the one from the latest source version, and from the most recent belief within that, rather than each source's value in turn. +The chart beside the KPI still draws every source, so it can show more points than the KPI counted. + We aim to support a graphical tool to edit these KPIs in the future. For now, you can set them by editing the asset's `kpi_sensors_to_show` field in the properties page, which will validate that the format is correct and tell you what to change. Read more about the format below. diff --git a/flexmeasures/api/v3_0/assets.py b/flexmeasures/api/v3_0/assets.py index 33fe92af26..9a9abf78de 100644 --- a/flexmeasures/api/v3_0/assets.py +++ b/flexmeasures/api/v3_0/assets.py @@ -2098,13 +2098,17 @@ def get_kpis(self, id: int, asset: GenericAsset, start, end): kpis = [] for kpi in asset_kpis: sensor = Sensor.query.get(kpi["sensor"]) - # The beliefs the chart draws: one value per event, the most recent one. - # Aggregating belief rows instead would count a revision on top of what it revised, - # and would count each source separately when several report the same sensor. + # One value per event, which is what a KPI reduces. + # Aggregating belief rows instead would count a revision on top of the belief it revised, + # and would count each source separately when several report the same event, + # so that a total came out higher than anything anyone reported. + # Where several do report an event, the value is the one from the latest source version, + # and from the most recent belief within that. beliefs = sensor.search_beliefs( event_starts_after=start, event_ends_before=end, most_recent_beliefs_only=True, + one_deterministic_belief_per_event=True, ) # Count each event once, under the window it starts in. # The search also returns events that merely overlap the window, which the chart draws, diff --git a/flexmeasures/api/v3_0/tests/test_assets_api.py b/flexmeasures/api/v3_0/tests/test_assets_api.py index 4bd78a2cc3..07362d5eee 100644 --- a/flexmeasures/api/v3_0/tests/test_assets_api.py +++ b/flexmeasures/api/v3_0/tests/test_assets_api.py @@ -1910,6 +1910,155 @@ def test_kpi_window_honours_the_offset_it_is_given( assert total != shifted, "the assertion above only means something if these differ" +@pytest.mark.parametrize("requesting_user", ["test_admin_user@seita.nl"], indirect=True) +def test_kpi_counts_an_event_once_when_two_sources_report_it( + db, client, setup_api_test_data, setup_sources, requesting_user +): + """Two sources reporting one event are two claims about it, not two contributions to it. + + Summing them produced a number no source ever reported, and that no point on the chart showed. + The KPI now reduces one value per event, and these two sources are of the same version, + so the one that believed the event more recently is the one it counts. + """ + asset_type = ( + db.session.query(GenericAssetType).filter_by(name="battery").one_or_none() + ) + asset = GenericAsset( + name="kpi with two sources on one event", + generic_asset_type=asset_type, + account_id=requesting_user.account_id, + ) + db.session.add(asset) + db.session.flush() + sensor = Sensor( + name="kpi with two sources sensor", + generic_asset=asset, + event_resolution=timedelta(days=1), + unit="EUR", + ) + db.session.add(sensor) + db.session.flush() + + sources = list(setup_sources.values()) + reported, corrected = sources[0], sources[-1] + assert reported.id != corrected.id, "this test needs two distinct sources" + + window_start = datetime(2030, 3, 15, tzinfo=utc) + db.session.bulk_insert_mappings( + TimedBelief, + [ + # One event, claimed by two sources, the second more recently than the first. + dict( + event_start=window_start, + belief_horizon=timedelta(days=2), + event_value=100.0, + sensor_id=sensor.id, + source_id=reported.id, + cumulative_probability=0.5, + ), + dict( + event_start=window_start, + belief_horizon=timedelta(days=1), + event_value=80.0, + sensor_id=sensor.id, + source_id=corrected.id, + cumulative_probability=0.5, + ), + ], + ) + asset.sensors_to_show_as_kpis = [ + {"title": "Daily costs", "sensor": sensor.id, "function": "sum"} + ] + db.session.flush() + + total = _kpi_total( + client, + asset, + window_start.isoformat(), + (window_start + timedelta(days=1)).isoformat(), + ) + assert total == pytest.approx( + 80.0 + ), "the more recent belief about the event, rather than 180.0, which neither source reported" + + +@pytest.mark.parametrize("requesting_user", ["test_admin_user@seita.nl"], indirect=True) +def test_kpi_prefers_the_latest_source_version_over_the_most_recent_belief( + db, client, setup_api_test_data, requesting_user +): + """A newer version of a source wins the event, even when an older version believed it more recently. + + Version comes first because it says which code produced the value, + where the belief time only says when it was said. + """ + from flexmeasures.data.models.data_sources import DataSource + + asset_type = ( + db.session.query(GenericAssetType).filter_by(name="battery").one_or_none() + ) + asset = GenericAsset( + name="kpi with two source versions", + generic_asset_type=asset_type, + account_id=requesting_user.account_id, + ) + db.session.add(asset) + db.session.flush() + sensor = Sensor( + name="kpi with two source versions sensor", + generic_asset=asset, + event_resolution=timedelta(days=1), + unit="EUR", + ) + db.session.add(sensor) + # Two versions of one reporter, which is what a release upgrade leaves behind. + older_version = DataSource( + name="Reporter", type="reporter", model="Rep", version="1" + ) + newer_version = DataSource( + name="Reporter", type="reporter", model="Rep", version="2" + ) + db.session.add_all([older_version, newer_version]) + db.session.flush() + + window_start = datetime(2030, 4, 15, tzinfo=utc) + db.session.bulk_insert_mappings( + TimedBelief, + [ + # The newer version spoke first, and the older version spoke later. + dict( + event_start=window_start, + belief_horizon=timedelta(days=2), + event_value=42.0, + sensor_id=sensor.id, + source_id=newer_version.id, + cumulative_probability=0.5, + ), + dict( + event_start=window_start, + belief_horizon=timedelta(days=1), + event_value=99.0, + sensor_id=sensor.id, + source_id=older_version.id, + cumulative_probability=0.5, + ), + ], + ) + asset.sensors_to_show_as_kpis = [ + {"title": "Daily costs", "sensor": sensor.id, "function": "sum"} + ] + db.session.flush() + + total = _kpi_total( + client, + asset, + window_start.isoformat(), + (window_start + timedelta(days=1)).isoformat(), + ) + assert total == pytest.approx( + 42.0 + ), "the newer version's value, despite the older belief time" + + @pytest.mark.parametrize("requesting_user", ["test_admin_user@seita.nl"], indirect=True) def test_kpi_reports_what_the_chart_draws( db, client, setup_api_test_data, setup_sources, requesting_user diff --git a/flexmeasures/data/models/time_series.py b/flexmeasures/data/models/time_series.py index 196fc1cd6f..a22a3d9c62 100644 --- a/flexmeasures/data/models/time_series.py +++ b/flexmeasures/data/models/time_series.py @@ -902,6 +902,10 @@ def _select_latest_version_and_belief_per_event( """Keep, per event, the single belief with the latest source version, breaking version ties by most recent belief time. + Beliefs that tie on both keep the order they came in, + which is what lets a caller express its own precedence by the order in which it passes its sources. + See `test_source_transition`, where the first source in the list wins the events both sources report. + Assumes deterministic beliefs (probabilistic depth 1) and a belief_time index level. """ source_codes, unique_sources = pd.factorize(bdf.index.get_level_values("source"))