Skip to content

fix(plan): stop tweak and second pass regressing the plan - #4402

Merged
springfall2008 merged 2 commits into
mainfrom
fix/monotonic-plan-passes
Jul 31, 2026
Merged

fix(plan): stop tweak and second pass regressing the plan#4402
springfall2008 merged 2 commits into
mainfrom
fix/monotonic-plan-passes

Conversation

@springfall2008

Copy link
Copy Markdown
Owner

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_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.

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 -> Demand for 16 minutes. should_replace_plan could not catch it — it compares the new plan against the previous cycle's plan, so the value tweak_plan had just destroyed was never visible to it. Two days of production logs showed 25 of 104 tweak_plan calls regressing the plan (24%).

Why it happens

Neither optimiser is blind to the status quo — optimise_charge_limit explicitly forces the current setting into its candidate list, and optimise_export regenerates 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:

  • The ranking score is not the plan metric. optimise_export applies the in-progress commitment bonus and -0.002/-0.001 tie-break weightings; optimise_charge_limit has 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 after minutes_now, so "stop now, restart later" got the same bonus as "keep exporting" and the two cancelled out.
  • The reference point is a fixed default. Both latch their basis on their first candidate — charge-to-max, or export-off — so every window is re-decided as though fresh.
  • The export gate is a cost test while the plan is judged on metric. Deliberate (Discharging early @15p at the expense of importing later at 30p. #2984), but it means a metric-improving export can be dropped.

metric_min_improvement defaults to 0.0 and metric_min_improvement_export to 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_n and neither optimiser touches self.*_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_pass gates 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 (<=), matching optimise_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_margin field (assigned 0.0 in fetch.py, 0 in predbat.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:

tie handling unchanged better worse avg metric
<= (this PR) 19 1 (-0.27) 0 -0.0136 better
strict < (tried, rejected) 18 1 (-0.27) 1 (+2.52) +0.1125 worse

In the one scenario that regressed under strict <, tweak_plan itself 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 means predbat_debug_pre_saving1's expected plan needs no regeneration.

cases/random_results.json is re-baselined: it differs from main in exactly one scenario, the improvement above.

Tests

The shared harness in test_export_commitment.py built its Prediction before soc_kw/soc_max and the rates were assigned. Prediction snapshots 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:

  • both passes revert a window change that worsens the plan
  • an in-progress export is not restarted later inside its own window
  • the metric a pass reports equals a fresh whole-plan simulation, for the export and charge branches

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, Prediction built before config — 5/5 caught.

Full ./run_all green (305s, 0 failures) and ./run_pre_commit clean.

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

springfall2008 and others added 2 commits July 31, 2026 13:04
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>
Copilot AI review requested due to automatic review settings July 31, 2026 12:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_plan and optimise_full_second_pass revert any single-window mutation that worsens the whole-plan metric (ties kept).
  • Extend optimise_export / optimise_charge_limit to also return the unadjusted plan metric used for monotonicity gating, while retaining adjusted metrics for internal ranking.
  • Fix and expand test_export_commitment so 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 thread apps/predbat/plan.py
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)
@springfall2008
springfall2008 merged commit 9e4df48 into main Jul 31, 2026
3 checks passed
@springfall2008
springfall2008 deleted the fix/monotonic-plan-passes branch July 31, 2026 13:02
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants