Skip to content

feat(plan): show a rate range for merged plan cells, fix threshold wording - #4362

Open
chalfontchubby wants to merge 8 commits into
springfall2008:mainfrom
chalfontchubby:feat/plan-why-reason-rate-range
Open

feat(plan): show a rate range for merged plan cells, fix threshold wording#4362
chalfontchubby wants to merge 8 commits into
springfall2008:mainfrom
chalfontchubby:feat/plan-why-reason-rate-range

Conversation

@chalfontchubby

@chalfontchubby chalfontchubby commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

Two small fixes to the "why" tooltip reason text, both from live-production feedback on #4311:

1. Merged/rowspan cells only showed the first slot's rate. When consecutive plan rows share the same state and get visually merged into one cell, the tooltip cited only the first slot's rate even when the actual rate varied across the span. Turns out this doesn't need any restructuring: rowspan and the window's own end minute are already computed at the exact point a span starts, and the per-minute rate data (self.rate_import/self.rate_export) is already fully populated by then - so the span's real min-max range can be computed right there. Added Output.rate_range_text(), wired into the rate-carrying reason codes (freeze_charge, charge_low_rate, export_high_rate). A merged cell now shows e.g. "15.00-25.00" instead of just the first slot's value; a single-slot cell is unaffected.

2. "vs. your {threshold}p/kWh threshold" wording overclaimed configuration. The threshold isn't something a user sets directly - it's calculated by the optimiser from the actual rates of the windows it selected (find_price_levels). Reworded to "vs. the calculated {threshold}p/kWh threshold" so it doesn't imply a setting that doesn't exist.

Also added since the original summary above (see PR comments for each)

  • Column-header hover tooltips (e.g. "XLoad kWh") explaining what each plan-table column means.
  • freeze_export reworded and its threshold reference removed entirely, not just reworded. The original "export rate Xp vs. the calculated Yp threshold" wording implied a hard rate-vs-threshold rule the code doesn't actually enforce - checked prediction.py directly: at limit==99 charging is zeroed and discharge-to-export is gated to limit < 99.0, so during a freeze the battery does neither; surplus solar just has nowhere to go but export. Reworded to state that mechanism directly, and the reason code was renamed from freeze_export_below_threshold to freeze_export (matching freeze_charge) since it no longer carries a rate/threshold param at all - it's not one of the rate-range-carrying codes from point 1 above anymore.
  • Split-cell tooltips now state the exact clock time the export window starts, and join both halves with a lowercase "Then " instead of a comma.

Test plan

  • Two new tests in test_plan_why_reason.py: a merged export cell spanning 3 slots at differing rates shows the correct min-max range in both the reasons[].params and the rendered tooltip text; a single-slot cell still shows a plain value, not a spurious range
  • ./run_all --test plan_why_reason --test plan_json_rate_adjust --test web_charts - all pass
  • pre-commit (ruff, black, cspell) run against the changed files, passed

🤖 Generated with Claude Code

…rding

Merged/rowspan cells (consecutive slots sharing the same state) only ever
showed the first slot's rate in the tooltip, even when the underlying
rate varied across the span. rowspan and the window's own end minute are
already known at the point a span starts, and the per-minute rate data is
already fully populated by then too, so the span's actual min-max range
can be computed in the same place rowspan already gets computed, with no
restructuring needed - falls back to a single value when the span is one
slot or the rate doesn't vary across it.

Also: "vs. your {threshold}p/kWh threshold" implied a user-configured
setting, but the threshold is calculated by the optimiser from the
selected windows' own rates (find_price_levels), not something set
directly - reworded to "the calculated {threshold}p/kWh threshold".
Column headers like "XLoad kWh" aren't self-explanatory to non-technical
users. Added a title= tooltip to every plan-table <th>, condensed from
the existing column-by-column descriptions in predbat-plan-card.md
(short, hover-appropriate summaries rather than the full colour-coding
detail from the docs page). New th(key, innerHtml) helper wires a
COLUMN_HEADER_HELP lookup into each header consistently.
@chalfontchubby

Copy link
Copy Markdown
Collaborator Author

Added one more thing to this PR: hover tooltips on the plan table's column headers themselves (e.g. "XLoad kWh"), not just the per-cell state reasons. Condensed from the existing column-by-column descriptions already in predbat-plan-card.md, wired via a new th(key, innerHtml) helper + COLUMN_HEADER_HELP lookup in get_plan_renderer_js(). Covered by a new test asserting every rendered header has a corresponding help entry.

🤖 Generated with Claude Code

…hold

freeze_export_below_threshold implied a rate-vs-threshold cutoff the code
doesn't actually enforce (same overclaim pattern already flagged for
FrzChrg). Checked prediction.py directly: at limit==99, charging is
zeroed (line ~790) and the discharge-to-export path is gated to
limit < 99.0 (line ~818), so the battery does neither - solar surplus
just has nowhere to go but export, at zero round-trip loss, while
actively discharging to sell more isn't worth the loss this slot. Reworded
to state that mechanism directly instead of a number-vs-number comparison
that was never how the decision actually gets made. Renamed the reason
code from freeze_export_below_threshold to freeze_export (matching
freeze_charge) since the new wording no longer references a threshold,
and dropped the now-unused rate/threshold params.
@chalfontchubby

Copy link
Copy Markdown
Collaborator Author

One more fix in this PR: reworded the FrzExp reason text. It previously implied a rate-vs-threshold cutoff ("export rate Xp vs. the calculated Yp threshold") that the code doesn't actually enforce as a hard rule - same overclaiming pattern already flagged elsewhere for FrzChrg. Checked prediction.py directly: at limit==99 charging is zeroed and the discharge-to-export path is gated to limit < 99.0, so the battery does neither during freeze - solar surplus just has nowhere to go but export (zero round-trip loss), while actively discharging to sell more isn't judged worth the loss that slot. Reworded to state that mechanism directly, and renamed the reason code from freeze_export_below_threshold to freeze_export (matching freeze_charge) since the new wording no longer references a threshold at all.

🤖 Generated with Claude Code

A split cell's pre-window reason said "Until the export window starts
partway through this slot" without saying when - the export window's
own start minute was already in scope at that point, just not
threaded through. Added {split_time} to the demand_before_export_*
templates, formatted the same way output.py already formats the row's
own timestamp (midnight_utc + timedelta).

Also joins the two halves into one narrative instead of two disconnected
sentences: renderReasonText() (and its Python test mirror) now prefixes
"Then " (no comma) onto the second half specifically when the first is a
demand_before_export_* code, lowercasing its first letter so it reads as
a continuation rather than two capitalised sentences.
@chalfontchubby

Copy link
Copy Markdown
Collaborator Author

Also added the split-time + "Then" transition suggested on the split-cell backlog item: the pre-window half of a split cell now states the exact clock time the export window starts (e.g. "Until 16:15, the battery level is expected to rise from solar generation.") instead of just "partway through this slot", and the second half is joined with a lowercase "Then " (no comma) so the two halves read as one narrative rather than two disconnected sentences.

🤖 Generated with Claude Code

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 adjusts the plan-table “why” tooltip rendering and related plan JSON generation, focusing on clearer tooltip narratives (split cells) and more accurate rate/threshold presentation for merged plan cells.

Changes:

  • Add column-header title= help tooltips and improve split-cell narrative joining (“Then …”) in the plan-table renderer JS.
  • Add Output.rate_range_text() and use it to emit min–max rate ranges for merged (rowspan) plan cells.
  • Update “why reason” templates/params and extend unit tests around tooltip rendering and merged-rate behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
apps/predbat/web_helper.py Adds column-header help tooltips and tweaks reason-text rendering for split cells (“Then …”).
apps/predbat/tests/test_plan_why_reason.py Updates the local mirror of reason rendering and adds coverage for merged export rate ranges + header tooltips.
apps/predbat/output.py Adds rate-range formatting helper, emits split-time params, and updates reason templates/params for plan tooltips.

Comment thread apps/predbat/output.py Outdated
Comment thread apps/predbat/output.py
springfall2008 and others added 3 commits July 29, 2026 15:34
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The Copilot Autofix commit on this PR fixed rate_range_text() defaulting
a minute missing from the rate dict to 0 instead of the given
fallback_value - that gap wasn't covered by the existing merged-cell
tests, which always populated every minute in the span. Added a case
that deletes a minute from rate_export mid-span and asserts the range
uses the row's own known rate, not a spurious 0. Verified this test
fails without the fix (0.00-20.00 instead of 20.00).

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (3)

apps/predbat/output.py:45

  • The PR description says the new rate-range helper is wired into the export-freeze threshold reason (freeze_export_below_threshold) and rewords its threshold wording. In this diff that reason code/template is removed and replaced with freeze_export that carries no rate/threshold params. If this is intentional, the PR description should be updated to match the implemented behavior.
    "freeze_charge": "Freeze charging — the battery holds at the current level rather than charging further this slot (import rate {rate}p/kWh vs. the calculated {threshold}p/kWh threshold).",
    "hold_charge_at_target": "Holding — the battery is already predicted to be at or above the {target_percent}% target for this window without charging further.",
    "charge_low_rate": "Charging up to {target_percent}% at the import rate for this slot of ({rate}p/kWh).",
    "freeze_export": "Freezing export — solar surplus passes straight to the grid, but it's not worth discharging the battery to sell more this slot.",
    "hold_export_unreachable": "Export window active but not triggered — the battery isn't predicted to reach the {target_percent}% level needed to export this slot.",

apps/predbat/web_helper.py:6429

  • COLUMN_HEADER_HELP hard-codes “pence per kWh”, but the app supports other minor currency symbols via currency_symbols (e.g. cents). This makes the header tooltips incorrect for non-GBP configs.
            const COLUMN_HEADER_HELP = {
                time: 'Predbat plans in slots (30 minutes by default) aligned to rate change times.',
                import: 'The import rate for this slot, in pence per kWh. Bold if a charge is planned this slot.',
                export: 'The export rate for this slot, in pence per kWh. Bold if a discharge/export is planned this slot.',

apps/predbat/web_helper.py:6449

  • th() expects extraAttrs to include a leading space (e.g. ' colspan="2"'), which is easy to misuse and would generate invalid markup like '<thcolspan=...>'. It’s safer to normalise/trim and add the space inside th().
            function th(key, innerHtml, extraAttrs) {
                const helpText = COLUMN_HEADER_HELP[key];
                const titleAttr = helpText ? ` title="${escapeAttr(helpText)}"` : '';
                return `<th${extraAttrs || ''}${titleAttr}><b>${innerHtml}</b></th>`;
            }

…s, harden th() extraAttrs

Per Copilot review on springfall2008#4362:
- COLUMN_HEADER_HELP hardcoded "pence per kWh" for the import/export
  column tooltips, which is wrong for non-GBP currencies. Now interpolates
  the same currencyMinor symbol already used in the header labels
  themselves (e.g. "Import p"), rather than spelling out an English
  currency name.
- th()'s extraAttrs required callers to remember a leading space or
  produce invalid markup (e.g. "<thcolspan=...>"). Now trims and adds
  the space inside th() itself.
@chalfontchubby

Copy link
Copy Markdown
Collaborator Author

Addressing the 3 points from the 2026-08-02 Copilot review:

  1. COLUMN_HEADER_HELP hardcoded "pence per kWh" - fixed in f652c1c: now interpolates the same currencyMinor symbol already used in the header labels themselves (e.g. "Import p"), instead of spelling out an English currency name that's wrong for non-GBP configs.
  2. th()'s extraAttrs needed a leading space or produced invalid markup - fixed in the same commit: th() now trims and adds the space itself, so a caller passing 'colspan="2"' or ' colspan="2"' both work correctly.
  3. PR description says the rate-range helper is wired into a reworded freeze_export_below_threshold, but the diff removes that reason and replaces it with freeze_export carrying no rate/threshold params - this is intentional, not a stale description. Explained in the 2026-07-29T08:28:40Z comment on this PR: the original threshold-referencing wording implied a hard rate-vs-threshold rule the code doesn't actually enforce (checked prediction.py directly - at limit==99 charging is zeroed and discharge-to-export is gated to limit < 99.0, so during a freeze the battery does neither; surplus solar just has nowhere to go but export). The reason code was renamed from freeze_export_below_threshold to freeze_export specifically because the new wording no longer references a threshold at all - so removing the params was the fix, not an oversight. Will tighten the PR description's wording to make that explicit rather than reading as "reworded" when it was actually "removed".

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

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.

3 participants