fix(mpc): cost every challenger with Core's own model - #1036
Conversation
…nch ask Python the same question The shadow's verdict could not be checked. Production said Python beat Core by ~97 öre on 55 of 55 replans; the replay bench, on a snapshot pulled from that same box the same day, said Python cost 32 öre MORE. Same plans, opposite signs, and no way to tell which number to believe. The bench was wrong. Its ExternalOptimizerConfig omits CVaRWeight while the box runs cvar_weight 0.15 at alpha 0.9, so the two were asking Python different questions. Setting the box's value in the bench reproduces the recorded figure exactly: -101.14 öre against the box's -101.14. The field measurement was right all along. So the numbers stand, but nothing forced them to. plan.TotalCostOre arrives from the optimizer and is only checked by ValidatePlan; the comparison never recomputed it from Core's own arithmetic, and Python's real objective (Solver.ObjectiveOre, 5003 öre against the 5453 the plan costs) was never in the comparison at all, which is what made the mismatch so hard to read. evaluatePlanOre now costs a given action sequence with the DP's own forward pass -- one copy of the arithmetic, shared through stepPlanSlot and stepLoadpointSoC, so a later change cannot silently invalidate the measurements. It refuses rather than guesses: wrong action count, non-finite values, slot misalignment, SoC out of band, power past limits. The shadow costs both plans this way, warns when Core's plan disagrees with Core's own bookkeeping, and records the self-reported figures beside the honest one. The bench takes the box's solver settings through FTW_MPC_BENCH_CVAR_WEIGHT / _CVAR_ALPHA / _MIP_GAP, so it can be pointed at the question production actually asks. Measured across 13 snapshots: the evaluator's drift from the plans' recorded totals is 0.000000000 öre, freshly solved and box-recorded alike, so the production verdict is unchanged to within 0.05 öre. Also surfaces a curtailment asymmetry found on the way: annotateCurtailment sets PVLimitW after TotalCostOre is summed, so a curtailing DP plan is costed as if it exported everything while ValidatePlan costs Python's with its curtailment. None of these snapshots has a negative spot slot, so it is not behind the gap above. Both sides are now costed at full PV and pv_curtailment_slots counts the case instead of leaving it silent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C1d4uknzs7QUv7nE9rW4fi
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3393c6a85
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if f, err := strconv.ParseFloat(v, 64); err == nil { | ||
| return f | ||
| } | ||
| } | ||
| return fallback |
There was a problem hiding this comment.
When any FTW_MPC_BENCH_* value contains a typo or non-numeric value, ParseFloat fails and this helper silently substitutes the fallback. In particular, a malformed CVaR weight becomes zero, recreating the configuration mismatch that this benchmark is intended to detect and potentially reversing the reported Python-vs-DP result while the log presents the fallback as the chosen setting. Fail the benchmark with the parse error instead of producing a misleading measurement.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } | ||
| if next > ceiling+planEvaluationLoadpointSoCTol { | ||
| return planEvaluation{}, fmt.Errorf( | ||
| "slot %d drives loadpoint %s to soc %.4f, above %.4f", i, lp.ID, next, ceiling) |
There was a problem hiding this comment.
Loadpoint band rejects valid Core plans
Medium Severity
evaluatePlan refuses a loadpoint walk that lands more than 0.0005 past SoCMax. The DP looks its EV policy up on a coarse SoC grid and applies that action to a continuous SoC, so a legal Core plan can overshoot the ceiling by up to half an EV grid step — typically far more than planEvaluationLoadpointSoCTol. The battery path already grants one DP grid step via socBandTolerance for this reason; the loadpoint path does not. On a site with a plugged-in EV, Core can then decline to cost its own published plan and the shadow comparison is dropped.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c3393c6. Configure here.
| SoCMax: 0.9, TargetSoC: 0.8, TargetSlotIdx: 20, Levels: 11, | ||
| MaxChargeW: 11000, ChargeEfficiency: 0.9, SurplusOnly: true, | ||
| } | ||
| return p, evaluationSlots(24) |
There was a problem hiding this comment.
Identity tests skip active loadpoints
Low Severity
The identity cases named with an active loadpoint and loadpoint on surplus only never set PluggedIn. LoadpointSpec.active() stays false, so both Optimize and evaluatePlan take the battery-only path. The sharing proof the PR relies on does not actually cover flex-load costing or the loadpoint SoC walk.
Reviewed by Cursor Bugbot for commit c3393c6. Configure here.
The shadow gap had to be attributable before anyone acted on it. The bench now scales each snapshot's TerminalSoCPrice through FTW_MPC_BENCH_TERMINAL_SCALE, applied before anything solves so the DP, Python and the correction all price stored energy the same way, and prints raw totals and end SoC for both plans beside the corrected ones. That is what the sweep needed, and the sweep settles the question: at a common 201x401 grid with no terminal credit, python - dp is -7.0 öre/plan across 13 snapshots, and on the only snapshot recorded under today's Core it is +39.4 in Core's favour. The -51.5 öre mean measured at each blob's own recorded resolution was the old 41x81 grid the Python era wrote, replayed faithfully. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C1d4uknzs7QUv7nE9rW4fi
Spot prices publish around 13:00. Before that the box holds roughly 12 hours of real day-ahead price; the rest of the 48 h horizon is an ML twin, blended toward the horizon mean by confidence. The owner asked whether guessing tomorrow earns anything. MPC dispatches only the first slot, so the question is not whether the guess moves a plan 30 hours out — it is whether it moves the watt that reaches hardware now. Two bench tests re-solve recorded /api/mpc/diagnose snapshots three ways on one grid: as recorded, with every forecast slot flattened to the horizon mean, and with the forecast slots deleted. They report the first slot's battery power, how many hours the plans stay together, and cost over the KNOWN window only, so all three are scored on the same real prices instead of one being judged on 48 h of guesses. The flattened variant uses confidence 1e-9, not 0. Core reads confidence <= 0 as "caller did not fill this in" and coerces it to 1.0, twice, so a variant written with a zero would silently measure total trust — the opposite of what it claims. A fixture test pins that coercion so the epsilon is not mistaken for fussiness. A second bench shortens the known window by hand — 2, 4, 6, 8, 12 h — leaving prices untouched, so it isolates the confidence rule from the twin's accuracy and finds where the guess starts to matter. Bench only; no planner code moves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C1d4uknzs7QUv7nE9rW4fi
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2a8686f. Configure here.
| exactAB, n, benchActionDiffW, sameAB, n, sumAbsAB/float64(n), maxAbsAB) | ||
| t.Logf(" first-slot W, C vs A: identical(<1W) %d/%d, within %.0f W %d/%d, mean |Δ| %.1f W, max |Δ| %.1f W", | ||
| exactAC, n, benchActionDiffW, sameAC, n, sumAbsAC/float64(n), maxAbsAC) | ||
| t.Logf(" mean hours before the plans part: A/B %.2f h, A/C %.2f h", sumDivAB/float64(n), sumDivAC/float64(n)) |
There was a problem hiding this comment.
Summary treats agreement as a split
Medium Severity
summarize always adds divABH / divACH to the “mean hours before the plans part” line, including rows where divABOK / divACOK is false. On agreement benchFirstDivergence returns the comparison length, so A/B becomes the full horizon (~48 h) and A/C the known window (~12 h). The per-row table prints none, but the summary — which this PR already says the three variants agree on — reports those sentinels as the time they parted.
Reviewed by Cursor Bugbot for commit 2a8686f. Configure here.
| sumDiffAC += float64(r.diffSlotsAC) | ||
| sumKnownN += float64(r.knownN) | ||
| sumSoCAC += r.cSoC - r.aSoC | ||
| sumSweepSoC += math.Abs(r.sweepSoC[1.5] - r.sweepSoC[0.5]) |
There was a problem hiding this comment.
Summary mixes refused rows as zero
Low Severity
summarize never consults costable. A refused known-window cost leaves aCorr/bCorr/cCorr and the SoC fields at 0, so those rows enter the mean B−A / C−A öre and end-of-window SoC lines as exact agreement. A failed terminal sweep stores NaN in sweepSoC, and math.Abs of that NaN poisons the SoC-spread summary for every snapshot.
Reviewed by Cursor Bugbot for commit 2a8686f. Configure here.
miravoss26
left a comment
miravoss26
left a comment
There was a problem hiding this comment.
Both champion and shadow plans in the Python-shadow diagnostic are now costed through Core's own forward pass (evaluatePlan, shared with the DP's own Optimize), instead of trusting each solver's self-reported total. Closes a real bug: the challenger's TotalCostOre is its own objective's opinion (CVaR-shaped, scenario-weighted) which isn't comparable to Core's DP cost — subtracting one from the other measured the objectives, not the plans. Adds refusal-as-a-result for infeasible/non-finite challenger plans and a self-consistency check on Core's own plan (ActiveEvaluationDriftOre), plus two new manually-run benches gated behind FTW_MPC_SNAPSHOT_DIR (not part of CI).
Findings:
- CI is red —
core (Go)/go test + vetboth fail onTestTheAppCannotMakeAnotherOwnerininternal/api(sql: database is closed). This looks unrelated to the diff: onlyinternal/mpcis touched, andinternal/mpc's own suite passes clean (ok ... internal/mpc 3.556s). Worth confirming it's flaky/pre-existing before merging on red CI — I can't tell from here whether it reproduces on main. - Minor — field semantics changed silently.
ShadowPlan.TotalCostOre(json:"total_cost_ore", on the/api/mpc/diagnosepayload) used to be the Python solver's self-reported cost; it's now Core's own evaluation of that plan. The old meaning moved to the newself_reported_orefield. If anything trendspython_shadow.total_cost_oreover time (dashboard/alert), expect a step-change at this deploy that isn't a real regime shift — just a metric redefinition. Might be worth a heads-up to whoever watches that number. - Security screen: clean. No secrets, no new dependencies (stdlib only), no injection surfaces, no authz changes, no new network destinations.
- Liked: sharing
stepPlanSlotbetween the DP's forward pass and the new evaluator is the right move — a second copy of that arithmetic would've silently invalidated every comparison the day one of them drifted. Test coverage is thorough (cost-equivalence to the DP's own report, every refusal path, plan-ranking sanity, and a dedicated test that a challenger can't buy a better verdict by lying about its own cost).
Needs a human decision on the CI failure before merge — my read of the diff itself is clean, but red CI is red CI.
2 participants
What this fixes
The shadow's verdict could not be checked. Production said Python beat Core by ~97 öre on 55 of 55 replans; the replay bench, on a snapshot pulled from the same box the same day, said Python cost 32 öre more. Same plans, opposite signs, no way to tell which to believe.
The bench was wrong, and the field number was right. The bench's
ExternalOptimizerConfigliteral omitsCVaRWeight, while the box runscvar_weight: 0.15, cvar_alpha: 0.9. The two were asking Python different questions. Sweeping it:Exact reproduction.
dp_corr − rec_corr = 0.0, so Core's side was already identical in both.Why the comparison still needed work
The numbers stand, but nothing forced them to.
plan.TotalCostOrearrives from the optimizer and is only checked byValidatePlan(±0.05 öre per slot); the comparison never recomputed it from Core's own arithmetic. And Python's real objective —Solver.ObjectiveOre, 5003 öre against the 5453 the plan costs — was never in the comparison at all, which is exactly what made the mismatch unreadable.evaluatePlanOrenow costs a given action sequence with the DP's own forward pass. The DP and the evaluator sharestepPlanSlot/stepLoadpointSoC— one copy of the cost arithmetic, so a later change to one cannot silently invalidate every measurement taken with the other. It refuses rather than guesses: wrong action count, non-finite values, slot misalignment, SoC out of band, power past limits.The shadow costs both plans this way, warns when Core's plan disagrees with Core's own bookkeeping, and records the self-reported figures beside the honest one. The bench takes the box's solver settings via
FTW_MPC_BENCH_CVAR_WEIGHT/_CVAR_ALPHA/_MIP_GAP.Measured across 13 snapshots, the evaluator's drift from the plans' recorded totals is 0.000000000 öre, freshly solved and box-recorded alike. The production verdict is unchanged to within 0.05 öre — it is now checkable rather than merely correct.
A second finding, surfaced not fixed
annotateCurtailmentsetsPVLimitWafterTotalCostOreis summed, so a curtailing DP plan is costed as if it exported everything, whileValidatePlancosts Python's plan with its curtailment. On a negative-spot day that biases the comparison toward Python. None of these 13 snapshots has a negative spot slot, so it is not behind the gap above. Both sides are now costed at full PV — symmetric, and it keeps Core's own number self-consistent — andpv_curtailment_slotscounts the case instead of leaving it silent. Worth its own look.Also noted:
planFromDiagnosticdropsp.Storagesandslot.Limits, so the bench replays a fleet box as a single aggregate battery with no fuse. Immaterial for these snapshots (one storage, limits never binding), but a snapshot is not yet a complete replay.What this means for the Core champion
The −97 öre gap is real, correctly measured, and not an artifact: Python genuinely beats the Go DP by 30–100 öre per plan on these snapshots, ~1.7% of plan cost. The plan to delete the Python optimizer should wait until that gap is understood — the most likely source is structural, not numerical: Core plans deterministically against a haircut PV forecast while Python solves against a scenario tree.
make verifygreen.Note
Medium Risk
Changes how Python shadow and replay metrics are computed (not live dispatch), but mistakes could misread champion vs challenger economics or hide real plan infeasibility; extensive new tests mitigate regression risk.
Overview
Python shadow and replay bench no longer trust each solver's self-reported
TotalCostOre. Both sides are scored by walking the action sequence through Core's sharedstepPlanSlot/evaluatePlanpath (same arithmetic as the DP), then applying terminal correction. Challenger claims are stored separately asself_reported_oreandself_reported_objective_ore; infeasible plans getevaluation_refused_reasonwith no diff. Core re-costing its own champion triggersactive_evaluation_drift_oreand a warning when it disagrees with what the DP reported.Diagnostics and benches gain parity and knobs. Shadow blocks add PV curtailment slot counts. Replay bench adds
FTW_MPC_BENCH_CVAR_WEIGHT,_CVAR_ALPHA,_MIP_GAP, optional terminal scaling, and columns for Python self-reported vs Core-evaluated costs. A new forecast-value bench re-solves snapshots three ways (as recorded, forecast flattened to horizon mean, forecast truncated) and compares first-slot battery power to see whether the price twin changes what hardware does now.Tests lock evaluator ↔ DP cost agreement, refusal paths, shadow behavior, and forecast-bench helpers (including the zero-confidence coercion trap).
Reviewed by Cursor Bugbot for commit 2a8686f. Bugbot is set up for automated code reviews on this repo. Configure here.