perf(plan): skip disabled windows in remove_intersecting_windows (3.1x faster planning) - #4505
Merged
Merged
Conversation
remove_intersecting_windows runs on every simulation - both Prediction. run_prediction and run_prediction_kernel call it before simulating - so it sits in front of the C++ kernel on the hot path. It scanned every charge window against every export window, testing "is this charge window enabled" and "is this export window enabled" inside the inner loop, so a plan carrying mostly disabled windows still paid the full O(charge x export) scan to do nothing. During optimisation that is the normal case, not an edge case. Instrumenting a benchmark scenario: 266 charge windows per call of which 24.6 enabled, 48.8 export windows of which 16.0 enabled - 139 million pair-iterations scanned per plan, of which 3.1% involved an enabled pair. Enabled export windows are now collected once per call (returning immediately when there are none, since nothing can clip), and a disabled charge window short-circuits instead of scanning every export window to discover it cannot be clipped. Both fast paths rebuild the window dicts exactly as the clipping path does, so the returned windows carry the same keys and are equally freshly owned. Profiled on the worst benchmark scenario, this function was 124.7s of a 152.9s plan (81%). After: that scenario drops from 152.0s to 25.6s. Across the 20 scenario benchmark, mean optimise time falls from 13.674s to 4.359s (3.1x) with plan metric and cost identical on all 20 - the clipping behaviour is unchanged, only the work skipped. Adds characterisation tests first: the fully-covered, disabled-charge, disabled-export, clip-start, clip-end and split cases, plus a randomised equivalence check comparing the implementation against a naive reference over 200 random window layouts. These pass before and after the change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR targets planner runtime by reducing hot-path overhead in remove_intersecting_windows, which is called on every simulation before the Python/C++ prediction kernels run. In addition to that optimisation, the PR also introduces model-based “dead slot” pruning and updates clipping/test expectations accordingly.
Changes:
- Optimise
utils.remove_intersecting_windows()by pre-filtering enabled export windows and short-circuiting disabled charge windows / no-enabled-export cases. - Add
nominal_onlysupport toPlan.run_prediction_metric()and introducePlan.prune_dead_plan_slots()to drop plan slots that are nominal-metric neutral. - Update/extend test suite and expected debug outputs (new prune tests, revised clip tests, random scenario clock/PV horizon adjustments).
Reviewed changes
Copilot reviewed 8 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
apps/predbat/utils.py |
Hot-path optimisation for intersecting-window clipping by skipping disabled windows. |
apps/predbat/plan.py |
Adds nominal-only metric path, introduces dead-slot pruning, and adjusts clipping behaviour. |
apps/predbat/unit_test.py |
Registers the new prune-dead-slots tests. |
apps/predbat/tests/test_window.py |
Adds characterisation + randomised equivalence tests for remove_intersecting_windows. |
apps/predbat/tests/test_random_scenarios.py |
Random scenario clock randomisation and PV forecast horizon fix. |
apps/predbat/tests/test_prune_dead_slots.py |
New tests covering nominal_only and prune_dead_plan_slots. |
apps/predbat/tests/test_clip_export_slots.py |
Updates tests to reflect clipping no longer removing certain windows (prune handles removal). |
apps/predbat/tests/test_clip_charge_slots.py |
Updates tests to reflect clipping no longer removing already-satisfied charge windows (prune handles removal). |
coverage/cases/predbat_debug_pre_saving1.yaml.expected.json |
Expected output updated for new plan behaviour. |
coverage/cases/predbat_debug_agile1.yaml.expected.json |
Expected output updated for new plan behaviour. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
springfall2008
force-pushed
the
perf-remove-intersecting-windows
branch
from
August 13, 2026 08:21
42c2f6b to
54dd46e
Compare
…esh random reference Removes the "clip again" loop. It re-ran the whole clipping pass over every charge window whenever a split left a tail long enough to keep, copying both window lists each time. With export windows processed in start order the retry cannot find anything: a head segment emitted by a split ends at the current export window's start, and every later export window starts at or after that, so nothing can reach back into it. Export windows are sorted here rather than assumed sorted, so correctness does not depend on the caller. This is not a speed-up - the benchmark is unchanged at 4.30s mean, so the retry was rarely triggering. It is a simplification and a latent bug fix: on unsorted input the old loop could emit a charge window overlapping an enabled export window and then fail to revisit it, because the retry was only armed when the remaining tail was at least 5 minutes long. Verified by differential testing the new implementation against the original from main over 300,000 random window layouts with sorted export windows (the invariant callers provide): zero mismatches. Repeating with deliberately unsorted export windows produces 495 disagreements in 200,000 layouts, and in every one it is the old implementation that leaves a charge window overlapping an enabled export. The in-repo randomised equivalence test now generates sub-5-minute windows, zero-length gaps and overlapping export windows, and runs 1000 layouts. Two faults in its naive reference surfaced as a result and are fixed: an unclipped window shorter than 5 minutes is kept rather than discarded, and windows that merely touch at a boundary overlap arithmetically but clip nothing, so they must not arm the minimum-length rule. Also refreshes cases/random_results.json, which run_random compares against. It was recorded on 2026-08-09 against the previous scenario set and was left stale when the scenarios were regenerated in #4491, so run_random reported large differences that were purely the scenario mismatch. Regenerated from the current scenarios; the plans are identical with and without this change, so the new reference is equally valid for main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
apps/predbat/utils.py:1043
- Inside the per-charge loop, once an export window fully clips the charge window (start >= end), the code still scans the remaining export_active entries. On large export_active lists this keeps doing unnecessary overlap checks, which is avoidable on this hot path.
# For each enabled discharge window, in start order
for dstart, dend in export_active:
# Overlapping window?
if (dstart < end) and (dend >= start):
if dstart <= start:
This was referenced Aug 13, 2026
Merged
springfall2008
added a commit
that referenced
this pull request
Aug 13, 2026
… (1.5x faster planning) (#4507) * perf(plan): cache export-to-charge window collisions in the optimiser optimise_charge_limit_price_threads asks hit_charge_window which charge window each export window collides with, once per export window per candidate, and hit_charge_window is a linear scan of the whole charge window list. On a benchmark scenario that was 1,741,593 scans of a ~200 entry list for 221 distinct answers - 27% of the plan's runtime, and the largest single cost after the intersect fix in #4505. The collision is purely geometric, and this function only ever turns windows on and off: it never moves a window's start or end. So the answer cannot change for the life of the call and is memoised in a local dict keyed by export window. Only the collision index is cached - the charge limit derived from it depends on charge_mods/best_limits_reset and still varies per trial. Scoping the cache to the call means there is nothing to invalidate. Confirmed by instrumentation before making the change: 1.74M calls, 221 distinct queries, and zero cases of a repeated query returning a different answer later. Random benchmark: mean optimise time 4.302s -> 2.896s (1.5x), worst scenario 25.6s -> 18.1s, with plan metric and cost identical on all 20 scenarios. Adds tests for hit_charge_window's contract - overlap, boundary-touch, and that the first matching window is returned, since the cache stores a specific index - plus a test asserting the invariant the cache rests on: run the optimiser and check the window bounds it was given come back untouched. If a future change starts moving windows, that test fails rather than the cache quietly returning stale collisions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(random): refresh the random benchmark reference for the collision cache cases/random_results.json is what run_random compares against, and it carries the recorded runtime of each scenario as well as its metric and cost. No seed differs on metric or cost - the cache does not change any plan - but the stored runtimes were recorded before it, so run_random reported a permanent ~1.4s of phantom improvement on every run. Regenerated: total recorded runtime 86.0s -> 60.2s, metric and cost unchanged on all 20 scenarios. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * [pre-commit.ci lite] apply automatic fixes --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com>
springfall2008
added a commit
that referenced
this pull request
Aug 13, 2026
The six per-simulation ctypes buffers were built as (ctypes.c_double * n)(*values), which unpacks the list into positional arguments - several times slower than copying from an array.array of the same type. Marshalling was the largest remaining cost in the planner after #4505, #4507 and #4508: 8.1s of a 13.9s profiled plan, more than everything else combined. Measured on the shapes the planner actually passes, building the charge window geometry drops from 35.0us to 12.7us per call. Content-keyed memoisation of the geometry arrays was measured as an alternative (10.8us) and rejected: it is only marginally ahead, because the key still has to be built and hashed, and it would have to stay correct across the passes that mutate window bounds in place. Reusing the soc_out buffer was also measured and is not worth it at 0.15us per allocation. from_buffer returns a view over the array.array rather than a copy, so the backing object has to outlive the kernel call. ctypes keeps it alive through the view's _objects; a new test asserts that rather than assuming it, since the failure mode is reading freed memory silently. The pool workers are separate forked processes so no buffer is shared between them, which was verified by running the same plan single-process and pooled and confirming identical results (not added as a test - it needs two full plan runs). Benchmark: worst scenario 11.03s -> 8.39s, mean optimise time across the 20 scenarios 3.717s -> 1.961s (1.9x), with plan metric and cost identical on all 20. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Merged
5 tasks
springfall2008
added a commit
that referenced
this pull request
Aug 13, 2026
….9x faster planning) (#4509) * perf(prediction): build the kernel's ctypes arrays via array.array The six per-simulation ctypes buffers were built as (ctypes.c_double * n)(*values), which unpacks the list into positional arguments - several times slower than copying from an array.array of the same type. Marshalling was the largest remaining cost in the planner after #4505, #4507 and #4508: 8.1s of a 13.9s profiled plan, more than everything else combined. Measured on the shapes the planner actually passes, building the charge window geometry drops from 35.0us to 12.7us per call. Content-keyed memoisation of the geometry arrays was measured as an alternative (10.8us) and rejected: it is only marginally ahead, because the key still has to be built and hashed, and it would have to stay correct across the passes that mutate window bounds in place. Reusing the soc_out buffer was also measured and is not worth it at 0.15us per allocation. from_buffer returns a view over the array.array rather than a copy, so the backing object has to outlive the kernel call. ctypes keeps it alive through the view's _objects; a new test asserts that rather than assuming it, since the failure mode is reading freed memory silently. The pool workers are separate forked processes so no buffer is shared between them, which was verified by running the same plan single-process and pooled and confirming identical results (not added as a test - it needs two full plan runs). Benchmark: worst scenario 11.03s -> 8.39s, mean optimise time across the 20 scenarios 3.717s -> 1.961s (1.9x), with plan metric and cost identical on all 20. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(prediction): guard the ctypes array typecode against a width mismatch Review feedback on #4509. array.array's integer typecodes are C types, so 'i' is a C int - 32 bit everywhere predbat runs, but not guaranteed to be. The failure mode is not an exception: from_buffer only checks the buffer is large enough, so a wider backing type is accepted and the kernel reads interleaved garbage. Confirmed directly - building a c_int32 array over an array('d') backing returns [0, 1072693248, 0] rather than raising. The typecode is now chosen at import by matching itemsize against the ctypes element, falling back to the slower (ctypes.c_double * n)(*values) construction if nothing matches, so a platform with unusual widths loses the speed-up rather than silently corrupting the simulation inputs. Also from the review: the retention check now uses truthiness via getattr rather than "is not None", since an empty _objects means nothing is retained and is just as unsafe as the attribute being absent; and the empty-input case is asserted for double_array as well as int32_array. Mutation-testing the new guard turned up a bug in the test itself: it asserted _objects retention unconditionally, but the fallback path copies its values and so has nothing to retain. On any platform taking the fallback the suite would have failed spuriously. The assertion is now scoped to the from_buffer path. Verified by forcing each state: correct typecodes pass, no typecode (fallback) passes, and a deliberately wrong-width typecode is caught by both the itemsize check and the round-trip check. 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.
Summary
remove_intersecting_windowsruns on every simulation — bothPrediction.run_prediction(prediction.py:529) andrun_prediction_kernel(prediction_kernel.py:470) call it before simulating — so it sits in Python, in front of the C++ kernel, on the hottest path in the planner.It scanned every charge window against every export window, testing "is this charge window enabled" (
limit > 0.0) and "is this export window enabled" (dlimit < 100.0) inside the inner loop. So a plan carrying mostly disabled windows still paid the full O(charge × export) scan to discover there was nothing to do — and during optimisation that is the normal case, not an edge case.Instrumenting one benchmark scenario:
97% of the work was scanning disabled windows.
The change
{start, end, average}dicts — preserving that matters because callers assign the result straight back overself.charge_window_best)The clipping logic itself is untouched; only skipped work changes.
Impact
Profiled on the worst benchmark scenario, this function was 124.7s of a 152.9s plan (81% of total runtime).
This is production-path code, so real installs should see the same reduction in plan computation time — most visible on tariffs that generate many windows (agile/negative-rate imports).
Test plan
--quick,debug_cases, pre-commit all passNote
Found while investigating why the random-scenario benchmark had become slow after the harness improvements in #4491. Independent of that PR and based on
main, so it can land on its own.🤖 Generated with Claude Code