fix(fetch): exclude saving-session/Axle boosted minutes from automatic rate thresholds (#5050)

[chalfontchubby]

Posted by Claude on behalf of @chalfontchubby.

Fixes #5050.

Problem

A saving session / Axle VPP event boosts both the import and export rate tables by the event reward, tagging those minutes "saving" in rate_import_replicated/rate_export_replicated (load_saving_slot() in octopus.py, load_axle_slot() in axle.py). In automatic threshold mode (rate_low_threshold=0), set_rate_thresholds() computed rate_import_cost_threshold from self.rate_max, which includes those event-boosted minutes.

On a two-rate tariff (25.95p day / 3.49p night) plus a +100p saving-session event, that pushed the threshold to 125.45p, and rate_scan_window() classified the whole ordinary-price day as "low rate" - binary_sensor.predbat_low_rate_slot stuck ON for ~24h, through the genuine 3.49p night window and the whole expensive day rate, matching the report exactly.

Fix

Added rate_minmax_excluding_saving() and used it for the threshold-stat inputs in set_rate_thresholds() only. self.rate_min/self.rate_max/self.rate_average (and the export equivalents) are deliberately left untouched - they feed dashboard sensors, graph scaling, and plan.py pricing, where the real boosted price is exactly what should be shown; this is a narrower, separate scan rather than a change to those.

This also resolves the triage's point 2 (the in-cycle auto-correction only fires after low_rates is already computed with the wrong threshold) as a side effect: rate_scan_window() now gets the correct threshold on the first pass, so self.low_rates - what downstream low-rate sensors, car-charging slots, and plan.py's candidate set consume - is correct from the start rather than needing the post-hoc correction to paper over it.

Verification

Reproduced against the exact triage numbers (48h two-rate tariff, +100p event 17:00-19:00, rate_low_threshold=0):

• Unfixed: rate_import_cost_threshold=125.45, the 25.95p day blocks come back as low-rate windows.
• Fixed: threshold=25.45 (true day rate - 0.5), only the genuine 3.49p night rate qualifies.

Three new tests in test_set_rate_thresholds.py, confirmed to fail against the unfixed wiring and pass with the fix:

• rate_minmax_excluding_saving returns the tariff's own min/max/average, skipping the boosted minutes

• falls back to the plain scan when an event covers the entire forecast window (no genuine minute to scan)

• end-to-end: set_rate_thresholds() + rate_scan_window() no longer misclassify the day rate, while rate_max itself stays untouched for other consumers

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

• ./run_pre_commit green

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Critical and moderate review findings remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes automatic rate-threshold misclassification caused by saving-session/Axle price boosts.

Changes:

• Excludes saving-event minutes from threshold statistics.
• Preserves boosted rates for dashboards and planning.
• Adds and registers regression tests.

File summaries

File	Summary	Findings
apps/predbat/unit_test.py	Registers the new tests.	—
apps/predbat/tests/test_set_rate_thresholds.py	Adds threshold regression coverage.	Moderate (3 votes): shared fixture state is not restored, making tests order-dependent.
apps/predbat/fetch.py	Adds saving-excluded rate statistics and applies them to thresholds.	Critical (1 vote): genuine 0p free sessions tagged saving are filtered out, potentially misclassifying rates (lines 2294 and 2396). Moderate (1 vote): export saving-event and manual-threshold coverage is missing.

Review details

Suppressed comments (2)

apps/predbat/fetch.py:2397

• The new tests exercise only an import boost; rate_export_replicated is empty and no case covers rate_high_threshold > 0. This branch changes both automatic and manual export threshold statistics, so add an export saving-event case that asserts the computed export threshold while leaving rate_export_max unchanged.

        rate_export_min, rate_export_max, rate_export_average = self.rate_minmax_excluding_saving(self.rate_export, self.rate_export_replicated)

apps/predbat/fetch.py:2397

• set_rate_thresholds() is also called when only one rate table exists (the fetch path guards with if self.rate_import or self.rate_export). For an empty side, rate_minmax_excluding_saving({}) falls back to (99999, 0, 0), so an import-only configuration now publishes an export threshold of 99998.9, and an export-only configuration compares against an import minimum of 99999 instead of the existing self.rate_* values. Filter only non-empty tables or retain the existing side's statistics when that table is absent.

        rate_min, rate_max, rate_average = self.rate_minmax_excluding_saving(self.rate_import, self.rate_import_replicated)
        rate_export_min, rate_export_max, rate_export_average = self.rate_minmax_excluding_saving(self.rate_export, self.rate_export_replicated)

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

[chalfontchubby]

(Posted by Rik; written by Claude.)

Copilot's follow-up review flagged two more findings, both as suppressed comments rather than inline threads.

Missing export-side / manual-mode test coverage - real gap, fixed in c435781. Every test here used rate_import_replicated only, with rate_high_threshold always at 0 (automatic mode) - set_rate_thresholds()'s manual branch multiplies rate_export_average directly by rate_high_threshold, so a boosted export minute contaminating that average was not caught by the automatic-mode min/max comparisons the existing tests already covered. Added a flat-15p-export-plus-50p-event scenario with rate_high_threshold=1.0; verified the contaminated/clean averages (~19.17p vs 15.0p exactly) before writing the assertion.

Empty-table fallback producing rate_export_cost_threshold = 99998.9 - confirmed the underlying oddity is real, but it predates this PR and is not a regression from it. Checked directly: rate_minmax_excluding_saving({}, {}) returns exactly (99999, 0, 0), identical to what the plain rate_minmax({}) on unmodified main already returns for an empty rate table - I diffed both and confirmed the values match byte for byte. set_rate_thresholds()'s manual-vs-automatic branching around rate_export_min/rate_export_max (the 99998.9 result) is unchanged by this PR either; I checked main's copy of that function and it produces the identical threshold for an import-only config today, before this diff ever touches it. Worth a maintainer's eye as its own issue in set_rate_thresholds(), but fixing it here would be out of scope for a PR about the "saving" tag's exclusion direction.

[Copilot]

🟡 Changes recommended

Moderate issues remain in reward filtering, empty-table handling, and test-state restoration.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

apps/predbat/fetch.py:2425

• Calling the helper for an empty export table changes the existing no-export behavior. rate_minmax({}) returns (99999, 0, 0), whereas the initialized export stats are all zero; fetch.py still calls set_rate_thresholds() when only import rates exist, so this makes the export branch produce a threshold of 99998.9 instead of the previous -0.1. Preserve the existing stored statistics when a rate table is empty (and likewise for import if that side is absent).

        rate_min, rate_max, rate_average = self.rate_minmax_excluding_saving(self.rate_import, self.rate_import_replicated)
        rate_export_min, rate_export_max, rate_export_average = self.rate_minmax_excluding_saving(self.rate_export, self.rate_export_replicated)

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

[Copilot]

🟡 Changes recommended

An unresolved production-path filtering issue and a critical test expectation issue remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

apps/predbat/fetch.py:2334

• This relies on the replicate reason remaining "saving", but the production fetch path applies basic_rates(..., prev=import_rates, rate_replicate=...) after loading saving/Axle sessions (fetch.py:1191-1195), and its rate_increment branch rewrites affected minutes to "increment" (fetch.py:2213-2215). The documentation explicitly supports using an export rate_increment during a saving session, so that supported combination bypasses this filter and leaves the boosted event price in the threshold statistics. Preserve the saving-event provenance separately from override tags, or otherwise distinguish the two sources.

            if rate_replicate.get(minute) == "saving":

apps/predbat/tests/test_set_rate_thresholds.py:289

• The missing comma concatenates these adjacent string literals into rate_high_thresholdalert_active_keep. The snapshot comprehension at line 306 then raises AttributeError before any of the registered tests run, so this suite cannot pass as written. Add the comma after rate_high_threshold.

    "rate_high_threshold"
    "alert_active_keep",

apps/predbat/tests/test_set_rate_thresholds.py:278

• The test helper also overwrites rate_import_base and rate_export_base at lines 50 and 53, but neither field is included in this snapshot. The finally block therefore leaves the synthetic 48-hour base curves on the shared PredBat instance, allowing later tests to observe leaked base-rate state. Include both fields in the snapshot.

    "rate_import",
    "rate_import_replicated",
    "rate_export",
    "rate_export_replicated",

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

[chalfontchubby]

(Posted by Rik; written by Claude.)

Addressed all 3 findings from the latest review:

fetch.py:2334 - production-path filtering gap: rate_minmax_excluding_saving() read rate_replicate.get(minute) == "saving" live, but the production fetch path calls load_saving_slot()/load_free_slot()/load_axle_slot() (tagging rate_replicate "saving") and then basic_rates()/apply_manual_rates() afterward - whose rate_increment branch overwrites that same minute's tag to "increment"/"user". That's a documented, supported combination (an override active during a saving session), and it silently let the boosted price back into the threshold stats since the "saving" provenance was already gone from rate_replicate by the time set_rate_thresholds() ran.

Fixed by capturing a frozen rate_import_saving_minutes/rate_export_saving_minutes set right after the three loaders run and before any override can touch rate_replicate, and reading that set instead. rate_replicate itself is untouched - output.py still needs its final tag for display. Added a dedicated test reproducing the override-overwrite scenario directly.

test_set_rate_thresholds.py:289 - missing comma crashed the whole test module: "rate_high_threshold" and "alert_active_keep" sat on adjacent lines in _SNAPSHOT_FIELDS with no comma, so Python's implicit string-literal concatenation merged them into "rate_high_thresholdalert_active_keep" - a nonexistent attribute, so the snapshot comprehension raised AttributeError before any test in the module could run. Confirmed the crash directly before fixing.

test_set_rate_thresholds.py:278 - rate_import_base/rate_export_base missing from the snapshot: the test helper sets both on the shared PredBat instance but neither was captured, so the group's restore left those synthetic 48h curves behind for every later test in the registry. Confirmed the leak directly (both differed from their original value after the group ran), fixed by adding them.

All three verified with mutation testing. Full ./run_all --quick + ./run_pre_commit green. Pushed as dc7153ad.

[Copilot]

🟡 Changes recommended

fetch.py has unresolved stale simulation state and an implementation-comment mismatch.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

apps/predbat/fetch.py:1201

• This explanation says rate_minmax_excluding_saving() reads rate_replicate live, but the helper explicitly consumes the frozen rate_import_saving_minutes/rate_export_saving_minutes set and never reads that dictionary. The contradiction makes the reason for taking the snapshot unclear and could lead a future change to remove the state protection; please align the comment with the implementation.

            # rate_minmax_excluding_saving()'s exclusion, since it reads rate_replicate live and
            # the "saving" provenance would already be gone by the time it runs (#5052 review).

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

[chalfontchubby]

(Posted by Rik; written by Claude.)

Fixed: the comment above the rate_import_saving_minutes snapshot was written to describe the problem before the fix existed, and never updated afterward - it still said the exclusion "reads rate_replicate live" and could be "fooled" by a later overwrite, when that's exactly what the snapshot now prevents. Reworded to describe the actual current behaviour: rate_minmax_excluding_saving() reads the frozen set precisely so a later overwrite can't erase the provenance it depends on.

Pushed as e0b9c060.

[springfall2008]

This seems over complex, why not just calc the min/max from the pre-adjusted rates (which are already stored)?