fix(plan): stop tweak and second pass regressing the plan - #4402
Merged
Conversation
Records the root cause analysis for tweak_plan and optimise_full_second_pass writing back window changes that make the whole plan worse, and the design for guarding both passes. Diagnosis, field logs and performance measurements originate from PR #4398 by @mbuhansen. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tweak_plan and optimise_full_second_pass call optimise_charge_limit / optimise_export one window at a time and write the result straight back with no check that the whole plan improved. Neither optimiser ranks candidates on the metric the plan is finally judged by: both score against a fixed default (charge to max, or export off) rather than the setting the plan already holds, and both rank on a score carrying adjustments the plan metric does not. The reported symptom was a force export at the best price of the horizon being cancelled mid-flight, then flapping Exporting -> Demand for 16 minutes. should_replace_plan could not catch it because it only compares against the previous cycle's plan, not against the value tweak_plan had just destroyed. Both passes now snapshot the single window they are about to change, and revert it unless the whole-plan metric did not worsen. The metric comes from the optimiser rather than a fresh simulation: it already simulates the entire plan for every option it tries, so the unadjusted metric of the option it selected describes the plan we now hold. Both optimisers therefore return that unadjusted metric alongside the adjusted one they rank on, leaving optimise_detailed_pass - which gates on the adjusted score - untouched. Ties are kept, matching optimise_swap_export. Reverting them was tried first and measured worse: a metric-neutral revert leaves a differently shaped plan of equal value, and the passes after tweak amplify that downstream. Also scopes the in-progress export commitment bonus to candidates that cover the current minute. Applying it to later-starting candidates too cancels it out, which is what let "stop now, restart later" win. Removes the dead best_soc_margin field, which applied after selection and would otherwise mean the SoC written back was not the one simulated. It is assigned 0.0 in fetch.py and 0 in predbat.py and set nowhere else. Tests: the shared harness built its Prediction before soc_kw/soc_max and the rates were set, so it simulated an empty battery, every plan cost 0.0 and any metric assertion was silently a no-op. Fixed, and guarded by a test. Adds coverage for both passes reverting a regression, the in-progress export not being restarted later in its window, and the reported metric matching a fresh whole-plan simulation for both branches. Random benchmark over 20 scenarios against main: 19 unchanged, 1 better, 0 worse. Diagnosis, field logs and the original fix are from PR #4398 by @mbuhansen. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens Predbat’s plan “tweak” and “full second pass” optimisation paths to be monotonic on whole-plan metric, preventing per-window optimisations from silently regressing an already-good plan (notably mid-flight forced exports).
Changes:
- Make
tweak_planandoptimise_full_second_passrevert any single-window mutation that worsens the whole-plan metric (ties kept). - Extend
optimise_export/optimise_charge_limitto also return the unadjusted plan metric used for monotonicity gating, while retaining adjusted metrics for internal ranking. - Fix and expand
test_export_commitmentso the harness simulates a non-empty battery and adds regression coverage for the monotonic guards and export commitment scoping.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/superpowers/specs/2026-07-31-monotonic-plan-passes-design.md | Design write-up documenting root cause and the monotonic-guard approach. |
| coverage/cases/random_results.json | Re-baselines random scenario golden results to reflect the new monotonic behaviour. |
| apps/predbat/tests/test_optimise_all_windows.py | Updates charge-limit optimiser call sites for the new return value. |
| apps/predbat/tests/test_export_commitment.py | Fixes the test harness ordering bug and adds regression tests for monotonicity and export commitment scoping. |
| apps/predbat/predbat.py | Removes dead best_soc_margin state from reset. |
| apps/predbat/plan.py | Implements monotonic window guards, returns plan-metric alongside adjusted metrics, scopes export commitment bonus, removes best_soc_margin behaviour. |
| apps/predbat/fetch.py | Removes dead best_soc_margin assignment from config fetch. |
Comment on lines
1725
to
+1726
| # Add margin last | ||
| best_soc = min(best_soc + self.best_soc_margin, self.soc_max) | ||
| best_soc = min(best_soc, self.soc_max) |
Merged
4 tasks
springfall2008
added a commit
that referenced
this pull request
Aug 12, 2026
prune_dead_plan_slots decides whether a slot achieves anything by asking the model, which makes the trajectory-based removal heuristics in clip_charge_slots and clip_export_slots redundant. Instrumenting every branch across the 20 scenario benchmark shows the removal branches fire 31 times with the prune disabled and 0 times with it enabled, while both clip-up branches keep firing unchanged (111 fires). The same holds on the real captures from #4453/#4478. Removed: freeze-export-at-100%, no-SoC-above-reserve (#4171/#4434), phantom export (#4453/#4487), export target-unreachable, and charge never-reaches-limit. What remains in both functions is limit adjustment only - narrowing a requested limit to what the window can actually achieve, so the target sent to the inverter matches the simulated plan and adjacent windows merge. The charge freeze-to-charge-at-100% conversion is kept: it rewrites a limit rather than removing a window, and covers windows the prune skips. The prune deliberately does not trial a window covering the current minute (#4402), so an in-progress phantom is no longer converted in place; it is re-planned on the next cycle from real inverter state instead. Random benchmark over 20 scenarios before vs after this removal: plan metric and cost identical 20/20, runtime unchanged. Debug cases pass unchanged - the expected files regenerated for the prune itself needed no further update, which is independent evidence the branches were dead. Tests that asserted the removed behaviour are rewritten to assert the new contract (clipping adjusts limits and never removes); the removal behaviour they covered is now exercised by the prune tests and the debug case regression. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
springfall2008
added a commit
that referenced
this pull request
Aug 12, 2026
The prune skipped any window covering the current minute, on the grounds that #4402 forbids cancelling an in-progress export. That over-applied the guard: #4402's regression came from writing back a change scored on optimise_export's adjusted metric (commitment bonus plus tie-break weightings) with no check that the whole plan improved, and its fix was to gate on the unadjusted whole-plan metric - exactly what the prune trial already uses. An in-progress export worth anything fails that gate and is kept. Skipping it also left the worst gap: the in-progress window is the slot being executed right now, so a dead one there is precisely the spurious command that reaches the inverter - and since the clip removal branches were dropped in favour of the prune, nothing covered it at all. Probing the 20 scenario benchmark, all 15 in-progress slots are worth real money to keep (+0.41p to +25.91p if removed), so none are pruned and the plans are identical 20/20 on metric and cost. Runtime +0.03s average for the extra trials. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
springfall2008
added a commit
that referenced
this pull request
Aug 13, 2026
…the central forecast (#4491) * feat(plan): model-based clipping - prune plan slots dead in the central forecast Follow-up to #4487. The heuristic clip branches each target one specific failure shape (battery pinned empty, target unreachable, SoC flat above limit), and #4453/#4478 showed new shapes keep appearing: every gate in the pipeline is tuned for a different pathology, and dead slots thread between them. Replace the guessing with a direct question to the model: remove the slot, re-simulate, did the central forecast change? prune_dead_plan_slots trials each active charge/export slot inside the record window in turn - the slot is removed and the whole plan re-simulated in the nominal (50%) scenario only, one simulation per trial via a new run_prediction_metric(nominal_only=True) option (skips pv10 and pv90). The removal is kept when the nominal metric does not get worse, so slots whose value exists only in the pessimistic branches - or nowhere at all - are dropped. If the pv10/pv90 conditions materialise in reality, the next plan recompute re-creates a genuine slot from actual state. The pass runs after the pre-clip scoring snapshot, so plan selection still compares plans as optimised (#4403). In-progress windows are never trialled (the #4402 commitment - the clip_export_slots phantom branch from #4487 remains as the complementary catch for that case), manual windows are preserved, and each accepted removal updates the running baseline so one removal cannot make the next look free. Random benchmark over 20 scenarios against main: 114 slots pruned, nominal cost of the executed plan never worse than +0.012p (the per-trial epsilon) and dramatically better on two scenarios (-35.18p, -71.61p - plans were carrying slots that cost real money in the central forecast for pessimistic -branch insurance); the pv-weighted metric of the executed plan gives back +0.62p on average (max +5.05p) where pure insurance was stripped, which is the designed trade. Plan runtime unchanged (1.33s avg both sides). Debug case expected files regenerated: agile1's pruned plan is 1.31p cheaper nominally (metric-neutral), pre_saving1's is cost-identical with a +0.21p pv10 residual. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(plan): drop the clip removal branches now subsumed by the prune prune_dead_plan_slots decides whether a slot achieves anything by asking the model, which makes the trajectory-based removal heuristics in clip_charge_slots and clip_export_slots redundant. Instrumenting every branch across the 20 scenario benchmark shows the removal branches fire 31 times with the prune disabled and 0 times with it enabled, while both clip-up branches keep firing unchanged (111 fires). The same holds on the real captures from #4453/#4478. Removed: freeze-export-at-100%, no-SoC-above-reserve (#4171/#4434), phantom export (#4453/#4487), export target-unreachable, and charge never-reaches-limit. What remains in both functions is limit adjustment only - narrowing a requested limit to what the window can actually achieve, so the target sent to the inverter matches the simulated plan and adjacent windows merge. The charge freeze-to-charge-at-100% conversion is kept: it rewrites a limit rather than removing a window, and covers windows the prune skips. The prune deliberately does not trial a window covering the current minute (#4402), so an in-progress phantom is no longer converted in place; it is re-planned on the next cycle from real inverter state instead. Random benchmark over 20 scenarios before vs after this removal: plan metric and cost identical 20/20, runtime unchanged. Debug cases pass unchanged - the expected files regenerated for the prune itself needed no further update, which is independent evidence the branches were dead. Tests that asserted the removed behaviour are rewritten to assert the new contract (clipping adjusts limits and never removes); the removal behaviour they covered is now exercised by the prune tests and the debug case regression. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(plan): let the prune trial in-progress slots too The prune skipped any window covering the current minute, on the grounds that #4402 forbids cancelling an in-progress export. That over-applied the guard: #4402's regression came from writing back a change scored on optimise_export's adjusted metric (commitment bonus plus tie-break weightings) with no check that the whole plan improved, and its fix was to gate on the unadjusted whole-plan metric - exactly what the prune trial already uses. An in-progress export worth anything fails that gate and is kept. Skipping it also left the worst gap: the in-progress window is the slot being executed right now, so a dead one there is precisely the spurious command that reaches the inverter - and since the clip removal branches were dropped in favour of the prune, nothing covered it at all. Probing the 20 scenario benchmark, all 15 in-progress slots are worth real money to keep (+0.41p to +25.91p if removed), so none are pruned and the plans are identical 20/20 on metric and cost. Runtime +0.03s average for the extra trials. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(random): start scenarios part way through a plan slot Every scenario ran from the template's minutes_now of 08:00, exactly on a 30 minute boundary, so a window covering the current minute always had its full length remaining. Partially-elapsed slots - the shape that matters for in-progress pruning and for anything that reasons about the remainder of the window being executed - were never generated. Scenarios now carry a clock offset sampled from 0/5/10/15/20/25 minutes (5 minutes being predbat's run cadence) and start that far into the slot. The offset is drawn last in the generator's random sequence, so re-generating an existing seed leaves every other parameter unchanged, and it is applied from the slot boundary rather than the current clock so that applying scenarios in a loop cannot accumulate offsets. Scenario files written before this carry no "clock" entry and keep the template's own minutes_now: re-running the committed cases/random_scenarios.yaml gives metric and cost identical to before on all 20, so previously recorded benchmark results stay comparable. A freshly generated set has 16 of 20 scenarios starting mid-slot and produces in-progress windows with 5, 15 and 25 minutes remaining (previously always 30). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(random): randomise scenario start time, and cover the whole PV horizon Replaces the within-slot clock offset with a random minute-of-day (still on predbat's 5 minute cadence), so scenarios start at every point of the tariff and solar day - overnight cheap windows, the evening peak, mid-generation - rather than all at the template's 08:00. Start times in the regenerated set span 00:10 to 22:10 with 15 of 20 landing off a 30 minute boundary, so partially-elapsed slots are now generated as a matter of course. Fixes a harness bug the randomisation would otherwise have made much worse: step_data_history reads forward series at (minute + minutes_now), but expand_pv_forecast only generated 0..forecast_minutes, so the tail of every horizon silently had no PV at all - 8 hours' worth even at the fixed 08:00 start, and nearly the whole horizon for a scenario starting late in the day. It now runs to minutes_now + forecast_minutes. cases/random_scenarios.yaml is regenerated, which resets the benchmark baseline: the scenarios are substantially richer (mean optimise time 1.3s -> 12.9s, because the horizon now actually contains solar), so results are not comparable with runs recorded before this commit. Re-measured on the new set, prune vs the pre-prune base: nominal cost better by 3.36p on average (best -43.21p, worst +0.06p, which is within the accumulated per-trial epsilon), pv-weighted metric +0.25p on average as pure pessimistic-branch insurance is stripped, optimise time +0.8s (6%). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.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.
Supersedes #4398 — same bug, same diagnosis, smaller and better-measured fix. The field logs, the root-cause analysis and the original implementation are @mbuhansen's work.
The bug
tweak_planandoptimise_full_second_passcalloptimise_charge_limit/optimise_exportone window at a time and write the result straight back, with no check that the whole plan improved.Reported symptom: a force export at the best price of the horizon was cancelled mid-flight and the inverter switched to Demand, then flapped
Exporting -> Demand -> Exporting -> Demandfor 16 minutes.should_replace_plancould not catch it — it compares the new plan against the previous cycle's plan, so the valuetweak_planhad just destroyed was never visible to it. Two days of production logs showed 25 of 104tweak_plancalls regressing the plan (24%).Why it happens
Neither optimiser is blind to the status quo —
optimise_charge_limitexplicitly forces the current setting into its candidate list, andoptimise_exportregenerates the full start grid. The current setting is always scored. It just isn't privileged, and it isn't scored on the yardstick the plan is finally judged by:optimise_exportapplies the in-progress commitment bonus and -0.002/-0.001 tie-break weightings;optimise_charge_limithas its own plus an isCharging bonus. Maximising that adjusted score can move the plan metric the wrong way. This is the direct cause of the reported symptom: the Sustain in-progress forced export across plan cycles (anti-flapping on flat peaks) #4118 commitment bonus was applied to every candidate including ones starting afterminutes_now, so "stop now, restart later" got the same bonus as "keep exporting" and the two cancelled out.metric_min_improvementdefaults to 0.0 andmetric_min_improvement_exportto 0.1, so hysteresis is a minor contributor. The ranking-score divergence is doing the damage.The fix
Both passes snapshot the single window they are about to change and revert it unless the whole-plan metric did not worsen. Every mutation in both passes is at index
window_nand neither optimiser touchesself.*_best, so a one-window restore is sufficient.No extra simulation. The optimisers already simulate the entire plan for every option they try, so the guard reuses that number rather than re-simulating. Both now return the unadjusted metric alongside the adjusted one they rank on —
optimise_detailed_passgates on the adjusted score and is deliberately untouched, since replacing it would strip the isCharging and export-commitment stickiness from the fresh-plan path. Verified on a real plan run: the reported metric matched a fresh whole-plan simulation on all 16 guard invocations, delta 0.0 on every one.Ties are kept (
<=), matchingoptimise_swap_export. Reverting them was tried first, on the reasoning that it reduces churn, and measured worse — see below.Also scopes the commitment bonus to candidates covering the current minute, and removes the dead
best_soc_marginfield (assigned0.0infetch.py,0inpredbat.py, set nowhere else; it applied after selection, so it would otherwise mean the SoC written back was not the one simulated).Measurements
Random benchmark, 20 scenarios, against
main:<=(this PR)<(tried, rejected)In the one scenario that regressed under strict
<,tweak_planitself finished at metric 256.76 / cost 384.81 under both rules — identical. The pass is monotonic either way; reverting a metric-neutral tie just left a differently shaped plan of equal value, and the passes after tweak amplified it. Keeping ties also meanspredbat_debug_pre_saving1's expected plan needs no regeneration.cases/random_results.jsonis re-baselined: it differs frommainin exactly one scenario, the improvement above.Tests
The shared harness in
test_export_commitment.pybuilt itsPredictionbeforesoc_kw/soc_maxand the rates were assigned.Predictionsnapshots those at construction, so it simulated an empty battery — every plan cost 0.0 and every metric assertion was silently a no-op. Fixed, and pinned by a test so it cannot regress.New coverage:
Each was verified to fail with its production change reverted. Mutation battery — guard disabled, bonus scoping reverted, export reports adjusted metric, charge reports adjusted metric,
Predictionbuilt before config — 5/5 caught.Full
./run_allgreen (305s, 0 failures) and./run_pre_commitclean.Not fixed here
The root causes above are treated, not cured: the passes will keep proposing regressions, we just stop accepting them. Fixing the ranking-score divergence would make them monotonic by construction and collapse the two metrics this PR has to carry back into one. Worth a follow-up issue.
🤖 Generated with Claude Code