fix(output): savings_today_predbat feeds the total with the same figure the sensor state shows (#3894)

[chalfontchubby]

Posted by Claude on behalf of @chalfontchubby.

Fixes #3894.

Problem

self.savings_today_predbat = saving used the unadjusted (real) figure, but it feeds three things that should all track the same quantity sensor.predbat_savings_yesterday_predbat's own state already publishes (saving_adjusted):

• savings_total_predbat (predbat.py), the running total sensor
• the predbat_savings_today_predbat Prometheus gauge
• the metrics dashboard's "Savings Yesterday (PredBat)" tile

So the running total accumulated positive real savings while the daily HA chart showed the adjusted (frequently negative) figure - a real, climbing total coexisting with daily bars sitting below zero, exactly the reported symptom.

The sibling metric, savings_today_pvbat, already used the adjusted figure correctly (self.savings_today_pvbat = saving_no_pvbat_adjusted, matching its own sensor's state=saving_no_pvbat_adjusted) - this was the one inconsistent case, not a deliberate distinction.

Scope

This PR is scoped to that one root cause. The original triage on this issue identified a second, larger defect: the battery-value adjustment bills the counterfactual battery's closing SoC level against the real one every day in perpetuity, rather than the day's change in level - so any export strategy that empties the battery overnight sees a permanent ~100p+/day penalty. That needs a redesign of the adjustment term (opening->closing delta on both sides), not a one-line fix, and is left for a separate PR. Two secondary issues from the same triage (savings_total_soc never clamped to soc_max; a stale-baseline window in the first ~2h after midnight) are likewise out of scope here.

Verification

The new test forces a real saving_real/saving_adjusted divergence (the default fixture has no battery-value adjustment to diverge on, so soc_kwh_history is set to a closing SoC that differs from the mocked prediction's final_soc) and reproduces the reported shape numerically: savings_today_predbat reads 400.0 (real) against a published 315.0 (adjusted) with the bug reintroduced, matching only with the fix in place. Confirmed with mutation testing - reverting the fix reliably fails the new test with that exact mismatch.

• ./run_all --quick green (310 tests)
• ./run_pre_commit green

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Address persisted-total migration and align the test’s SoC injection key with production lookup.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR aligns PredBat savings accumulation with the adjusted daily sensor value and adds regression coverage.

Changes:

• Uses saving_adjusted for savings totals and metrics.
• Adds real-versus-adjusted savings regression coverage.

File summaries

File	Summary
apps/predbat/output.py	Uses adjusted savings for downstream totals and metrics; persisted totals are not migrated.
apps/predbat/tests/test_calculate_yesterday.py	Adds regression coverage, but the injected SoC uses a key production does not read.

Review details

Suppressed comments (1)

apps/predbat/output.py:3497

• This changes the addend for future updates, but the total is persisted across upgrades: predbat.py loads the existing savings_total_predbat and then adds this value without any migration or reset. Users upgrading from the old code will therefore retain a total accumulated from unadjusted saving and append adjusted values afterward, so the total still does not represent the sum of the published daily states. Please either migrate/rebuild the persisted total (or explicitly start a new total at this definition change) before claiming the quantities are aligned.

        self.savings_today_predbat = saving_adjusted

• Files reviewed: 2/2 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.

[Copilot]

🔵 Needs a closer look

Address persisted-total migration and restore shared test history safely.

Review details

Suppressed comments (2)

apps/predbat/output.py:3497

• This only changes future increments; it does not repair an existing installation's persisted total. predbat.py:1196 loads the old savings_total_predbat, and :1249 only adds the new daily value, so a total accumulated under the old saving_real semantics remains mixed with adjusted daily history and can continue showing the reported mismatch after upgrade. Please add a one-time rebaseline/reset or otherwise explicitly handle totals created before this change.

        self.savings_today_predbat = saving_adjusted

apps/predbat/tests/test_calculate_yesterday.py:414

• This test runs on the single my_predbat instance shared by the test registry (apps/predbat/unit_test.py:818-823), so replacing the history with {} on cleanup can discard history that existed before this test and make later tests order-dependent. Save the pre-test soc_kwh_history before injecting the fixture and restore that object here (preferably from a finally block).

    my_predbat.soc_kwh_history = {}

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[chalfontchubby]

(Posted by Rik; written by Claude.)

Copilot flagged two further findings that did not surface as inline review comments (GitHub marked them "suppressed" - visible in the review body, not as separate threads). Both real.

soc_kwh_history = {} cleanup discarding shared state - fixed in ada9a6c. unit_test.py runs every registered test against one shared PredBat instance; the test's cleanup reset soc_kwh_history to {} unconditionally instead of restoring whatever an earlier test left there. Same pattern already fixed elsewhere this review round (#5052). Now snapshots before mutating and restores on cleanup - verified directly by seeding unrelated data beforehand and confirming it survives the test unchanged.

Persisted savings_total_predbat is not migrated - confirmed real, not fixed, and I do not think it should be fixed silently in this PR. predbat.py:1196 loads the existing total from HA state and only ever adds to it (:1249); this PR changes what gets added going forward but cannot retroactively un-mix what was already summed under the old saving (unadjusted) semantics. git log -S shows that line has been self.savings_today_predbat = saving since a very early commit (a2789970), so this is not a small recent contamination - it's the entire historical total for every existing installation.

The honest options are a one-time reset (loses the historical figure, but is exact from that point on), leaving it to self-correct gradually as new adjusted daily values dilute the old mixed total, or something in between (e.g. a changelog note telling anyone who cares to reset it by hand). All three are visible, somewhat irreversible changes to a number users see, and I don't think it's my call to make unprompted - flagging for @springfall2008 to decide the shape of, separately from this PR's actual bug fix.

[chalfontchubby]

@springfall2008 If the patch makes sense - thn we need to decide what to do about old data

[springfall2008]

The total savings is meant to be based on real data, if you use adjusted its not a true savings.

This however sounds like something to look into

"The original triage on this issue identified a second, larger defect: the battery-value adjustment bills the counterfactual battery's closing SoC level against the real one every day in perpetuity, rather than the day's change in level - so any export strategy that empties the battery overnight sees a permanent ~100p+/day penalty. That needs a redesign of the adjustment term (opening->closing delta on both sides), not a one-line fix, and is left for a separate PR. Two secondary issues from the same triage (savings_total_soc never clamped to soc_max; a stale-baseline window in the first ~2h after midnight) are likewise out of scope here."

[springfall2008]

Changed the total which isn't desirable, at least not without good reason?

[chalfontchubby]

Ok - so this PR is the wrong shape - close it?

[chalfontchubby]

(Posted by Rik; written by Claude.)

Closing this - the premise doesn't hold up. @springfall2008's answer above is that savings_total_predbat is meant to track real money saved, not the adjusted figure, so making it match savings_yesterday_predbat's adjusted state was the wrong direction.

The actual root of #3894 looks like a documentation gap rather than a calculation bug: docs/output-data.md described the total as summing the sensor's own displayed state, which it never has - I've corrected that separately. There's still an open question of whether savings_yesterday_predbat's daily displayed state should switch to the real figure (to visibly reconcile with the total), which I've raised on the issue for a decision.