Repository navigation
Write PV Forecast to the daily log - #4270
Conversation
Reviewer's GuideData structuresAdd the current forecast value to the daily log entry so it can be persisted alongside the other measurements.
APIExpose a method that retrieves the most recent PV forecast value at or before the current time.
Uses of the APICall the forecast accessor while building each log entry, and fall back to no forecast if retrieval fails.
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="packages/helpermodules/measurement_logging/write_log.py" line_range="150" />
<code_context>
+ existing_forecast = content.get("forecast")
+ if not isinstance(existing_forecast, dict):
+ existing_forecast = {}
+ existing_forecast.update(get_forecast_values())
+ content["forecast"] = existing_forecast
write_and_check(filepath, content)
</code_context>
<issue_to_address>
**Daily entries disappear on invalid forecast**
When a truthy scalar or list reaches `today_values`, `get_forecast_values()` returns the value and `existing_forecast.update(...)` raises `TypeError`; `save_log` catches it and returns `None`, so the cycle writes no daily entry and downstream daily-yield updates receive no entries.
Validate that `get_forecast_values()` returns a mapping before merging it, and handle invalid values without aborting the daily entry write.
</issue_to_address>
### Comment 2
<location path="packages/helpermodules/measurement_logging/write_log.py" line_range="150" />
<code_context>
+ existing_forecast = content.get("forecast")
+ if not isinstance(existing_forecast, dict):
+ existing_forecast = {}
+ existing_forecast.update(get_forecast_values())
+ content["forecast"] = existing_forecast
write_and_check(filepath, content)
</code_context>
<issue_to_address>
**Forecast data can exhaust disk**
When an anonymous MQTT client repeatedly publishes `today_values` maps containing new keys, `save_log` merges every supplied key into the daily `forecast` map and never removes old keys, so repeated updates with new keys grow the persisted JSON without bound and eventually fill storage, disrupting writes across the device.
Validate forecast timestamps and cap the map’s size, pruning obsolete keys before persisting it.
</issue_to_address>
### Comment 3
<location path="packages/helpermodules/measurement_logging/write_log.py" line_range="150" />
<code_context>
+ existing_forecast = content.get("forecast")
+ if not isinstance(existing_forecast, dict):
+ existing_forecast = {}
+ existing_forecast.update(get_forecast_values())
+ content["forecast"] = existing_forecast
write_and_check(filepath, content)
</code_context>
<issue_to_address>
**Forecast lands in wrong day**
When the save spans midnight and the forecast’s today-values are refreshed between filename selection and the merge, `save_log` merges `get_forecast_values()` into the file whose date it captured earlier; if the forecast’s today-values are refreshed across midnight in between, timestamps for the new day are written into the previous day’s file, so the daily forecast history is assigned to the wrong date.
Use one captured date for both the log filename and selecting the forecast values to persist.
</issue_to_address>
### Comment 4
<location path="packages/helpermodules/measurement_logging/write_log_test.py" line_range="152" />
<code_context>
monkeypatch.setattr(write_log, "get_names", Mock(
return_value={"cp1": "Bestehender Ladepunkt", "cp2": "Neuer Ladepunkt"}))
monkeypatch.setattr(write_log, "get_colors", Mock(return_value={"cp1": "#111111", "cp2": "#222222"}))
+ monkeypatch.setattr(write_log, "get_forecast_values", Mock(return_value={"2": 200.0}))
write_and_check_mock = Mock()
monkeypatch.setattr(write_log, "write_and_check", write_and_check_mock)
</code_context>
<issue_to_address>
**Forecast lookup failures go unnoticed**
When the production forecast lookup is broken, if `get_forecast_values` reads the wrong path or returns an empty mapping, `test_save_log_keeps_existing_names_and_colors_when_writing_new_entry` still passes: line 152 replaces the real lookup with `{"2": 200.0}`, and the assertion at line 172 only verifies that mocked value was merged.
Exercise the real `get_forecast_values` lookup with forecast data provided through its actual source, or add a separate test for that function.
</issue_to_address>
|
"Daily entries disappear on invalid forecast" und "Forecast lookup failures go unnoticed" waren berechtigt und sind behoben: |
There was a problem hiding this comment.
Wäre es nicht genauer, wenn man immer nur die aktuelle Prognose erfasst?
Werden im Tagesverlauf auch Prognosewerte für vergangene Stunden nachträglich korrigiert, sieht die abends gespeicherte Kurve womöglich deutlich treffender aus, als die Prognose tatsächlich war, als die Regelung ihre Entscheidungen getroffen hat. Das wäre ein Rückschaufehler.
Selbst wenn der Anbieter zurückliegende Stunden nicht mehr korrigiert, hat man mit der oben vorgeschlagenen Methode trotzdem noch einen Rückschaufehler, weil die Prognose möglicherweise ganz anders aussah, als die openWB morgens bei günstigen Netzpreisen die Ladung gestartet hat und im Log dann der Tagesverlauf der Prognose gespeichert wird.
Speichert man morgen von 6.00 die Prognose im Log, hat man die Verfeinerung über den Tag nicht. Und zu jeder Uhrzeit die Prognose vom restlichen Tag zu speichern, kann man nicht sinnvoll grafisch aufbereiten. Das sind zu viele Daten.
Ich bin dafür, den aktuellen Prognosewert zu loggen.
Unabhängig davon, sollte der Prognosewert mit in die Zeitstempel-Einträge und nicht als separates Dict geführt werden. Das sind Dictionaries, die Daten enthalten, die für alle Einträge gelten. Die zeitlichen Verläufe sind alle unter dem jeweiligen Zeitstempel abgelegt und werden anhand dessen grafisch aufbereitet. Die Strompreise werden ebenfalls unter den Zeitstempeln geloggt, da kannst Du dich für die Prognosedaten dran orientieren.
Du hast völlig recht ja, das hab ich nicht so ganz bedacht. Hab's umgesetzt: es wird jetzt nur noch der aktuelle Prognosewert geloggt (kein nachträglich verfeinerter Verlauf mehr), und zwar direkt im jeweiligen Zeitstempel-Eintrag, analog zu den Strompreisen. |
Summary by Sourcery
Record the current PV forecast alongside existing daily log data.
New Features:
Enhancements: