Skip to content

fix: cap DP terminal-value estimate at the best achievable in-horizon export price (replaces #245) #246

Description

@johanzander

Background

#244 reported that _calculate_terminal_value() (core/bess/battery_system_manager.py, ~line 1704) estimates the value of a kWh surviving past the DP's horizon boundary using the median buy price, and that this overvalues holding charge on contracts with a large buy/sell spread (Belgian ENTSO-e/Belpex, reported in #126) — the DP held the battery through the evening price peak instead of exporting, chasing a fictitious terminal bonus.

A first attempt (#245, closed without merging) simply swapped buy_prices for sell_prices in the same formula. bess-analyst review found this to be the wrong fix: since cycle cost is only ever charged on charging (never on discharge, per _compute_reward), a sell-median-based terminal value is structurally biased below the ordinary in-horizon export value for roughly half of any normal day. This isn't Belgium-specific — it's a property of the formula. Verified empirically: a realistic non-Belgian evening-peak scenario (buy 0.6 baseline / 1.4 peak, sell = 0.85×buy) drained the battery to near-empty (1.05 of 15 kWh usable) where the un-patched code correctly holds a substantial reserve (10.55 kWh). A straight swap trades Belgium's bug for a universal one.

Root-cause framing

A terminal value estimating "worth of a kWh surviving past the horizon boundary" is only valid if it never exceeds the best certain payoff already visible and actionable inside today's own horizon. If it does, the DP forgoes a known-good, in-horizon outcome (e.g. exporting at a real evening peak price) in favor of a fictitious future one — that's the actual bug in both the original code and in #245's fix.

Proposed fix

Keep the existing buy-median "avoid tomorrow's purchase" estimate (legitimate, and empirically correct for ordinary/Nordic-shaped markets), but bound it with an arbitrage-consistency cap derived from the DP's own visible horizon:

buy_based = max(0.0, statistics.median(buy_prices) * efficiency_discharge - cycle_cost_per_kwh)
sell_cap  = max(0.0, max(sell_prices) * efficiency_discharge - cycle_cost_per_kwh)
terminal_value = min(buy_based, sell_cap)

Same fix needs applying to the duplicated estimator in core/bess/simulation/verification.py (realized_under_solar_error), which independently reimplements the same formula.

Options considered and rejected

  • Straight sell-price swap (fix: base DP terminal value on sell price, not buy price #245) — rejected: proven to over-drain ordinary markets.
  • Extend the DP horizon into tomorrow with a provisional price forecast, dropping the terminal-value heuristic entirely — theoretically the cleanest fix, but a disproportionate lift for this bug (needs a forecast source for tomorrow before prices are known, a schedule-discontinuity question when real tomorrow prices land ~13:00, much larger review surface). Worth its own issue as a longer-term redesign, not a blocker here.
  • Spread-aware dampening with a hardcoded threshold — rejected as a strictly worse version of the cap: same qualitative effect, but with an arbitrary market-specific constant instead of a bound the DP already knows from its own horizon.

Regression tests required before merge

  1. Belgian-shaped unit test (reusing Belpex H integration? #126's real numbers) asserting terminal_value sits at/below the real evening peak's export value, and that optimize_battery_schedule actually discharges/exports during that peak — replaces fix: base DP terminal value on sell price, not buy price #245's test_based_on_sell_price_not_buy_price.
  2. New ordinary/Nordic-shaped regression test with a realistic diurnal buy/sell curve and a genuine narrow peak, asserting the battery still retains a substantial reserve at horizon end (this is the exact gap fix: base DP terminal value on sell price, not buy price #245 left untested, and the mechanism that caused its regression).
  3. Extend TestCalculateTerminalValue (core/bess/tests/unit/test_extended_horizon.py) with explicit hand-computed cases for both the buy-based and capped branches; keep the existing "horizon extends past today → 0.0" and floor-at-0.0 cases as-is.
  4. Apply identical fix to core/bess/simulation/verification.py, rerun test_plan_faithfulness.py.
  5. Full fast suite + slow suites test_extended_horizon.py / test_terminal_value.py.
  6. If feasible, replay Belpex H integration? #126's actual debug bundle via mock-run.sh to confirm the real reported symptom resolves against the user's real data, not just synthetic data.

Supersedes the approach in #245 (closed). Root issue: #244.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bot-analyzedTriage bot has processed this issuebugSomething isn't workingneeds-debug-logWaiting for user to provide debug export

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions