-
-
Notifications
You must be signed in to change notification settings - Fork 124
Fix savings_yesterday_predbat fluctuating throughout the day on dynamic tariffs #3881
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
f613d7e
Initial plan
Copilot 84d816e
Fix savings_yesterday instability: limit rate_low to yesterday's rate…
Copilot 30fb4f8
Simplify rate_low fallback logic to avoid nested ternary (code review)
Copilot 78efe99
Potential fix for pull request finding
springfall2008 da44d39
Refactor rate_low into helper method and fix test state management
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,150 @@ | ||
| # ----------------------------------------------------------------------------- | ||
| # Predbat Home Battery System | ||
| # Copyright Trefor Southwell 2026 - All Rights Reserved | ||
| # This application maybe used for personal use only and not for commercial use | ||
| # ----------------------------------------------------------------------------- | ||
| # fmt off | ||
| # pylint: disable=consider-using-f-string | ||
| # pylint: disable=line-too-long | ||
| # pylint: disable=attribute-defined-outside-init | ||
|
|
||
|
|
||
| def test_savings_stability(my_predbat): | ||
| """ | ||
| Test that the rate_low threshold computed in calculate_yesterday is derived only from | ||
| yesterday's rate range (k < end_record = 1440) and not from today's rates. | ||
|
|
||
| Bug: rate_low = min(past_rates.values()) included today's dynamically-added rates | ||
| (beyond minute 1440), so as minutes_now increased through the day and cheaper | ||
| today-rates entered the dict, the threshold dropped. That caused rate_scan_window to | ||
| find fewer (or no) charge windows for yesterday, producing a different metric_baseline | ||
| and thus a different savings_yesterday value on each hourly recalculation. | ||
|
|
||
| Fix: compute rate_low only from k < end_record (yesterday's window). | ||
| """ | ||
| failed = 0 | ||
| end_record = 24 * 60 # 1440 | ||
|
|
||
| # Build a set of "yesterday" rates (minutes 0..1439 relative to yesterday midnight) | ||
| # using a simple two-band Agile-like tariff: cheap 5p / expensive 25p | ||
| cheap_rate = 5.0 | ||
| expensive_rate = 25.0 | ||
| yesterday_rates = {} | ||
| for minute in range(0, end_record): | ||
| # Cheap rate 00:00-06:00 (minutes 0..359), expensive otherwise | ||
| yesterday_rates[minute] = cheap_rate if minute < 360 else expensive_rate | ||
|
|
||
| # Simulate how history_to_future_rates constructs past_rates: | ||
| # past_rates[k] = rate_import.get(k - 1440, 0.0) | ||
| # For k < 1440 we need rate_import to have entries at (k - 1440), i.e. negative keys. | ||
| # Build a rate_import dict with negative keys for yesterday. | ||
| rate_import = {} | ||
| for minute in range(0, end_record): | ||
| rate_import[minute - end_record] = yesterday_rates[minute] | ||
|
|
||
| # Today has a very cheap slot (0 p, like a negative Agile rate) at minutes 60..120 | ||
| today_cheap_rate = 0.0 | ||
| for minute in range(0, end_record): | ||
| rate = today_cheap_rate if 60 <= minute < 120 else expensive_rate | ||
| rate_import[minute] = rate | ||
|
|
||
| # Build past_rates as calculate_yesterday does, for two different values of minutes_now | ||
| def build_past_rates(minutes_now_val): | ||
| """Build past_rates covering end_record + minutes_now_val entries.""" | ||
| fut = {} | ||
| for k in range(0, end_record + minutes_now_val): | ||
| fut[k] = rate_import.get(k - end_record, 0.0) | ||
| return fut | ||
|
|
||
| # ---- Case 1: midnight (minutes_now=0) ---- | ||
| past_rates_midnight = build_past_rates(0) | ||
| yesterday_vals_midnight = [v for k, v in past_rates_midnight.items() if k < end_record] | ||
| rate_low_midnight = min(yesterday_vals_midnight) if yesterday_vals_midnight else 0.0 | ||
|
|
||
| # ---- Case 2: mid-morning (minutes_now=240) – today's 0p slot is now included ---- | ||
| past_rates_morning = build_past_rates(240) | ||
| yesterday_vals_morning = [v for k, v in past_rates_morning.items() if k < end_record] | ||
| rate_low_morning = min(yesterday_vals_morning) if yesterday_vals_morning else 0.0 | ||
|
|
||
| # ---- Case 3: noon (minutes_now=720) ---- | ||
| past_rates_noon = build_past_rates(720) | ||
| yesterday_vals_noon = [v for k, v in past_rates_noon.items() if k < end_record] | ||
| rate_low_noon = min(yesterday_vals_noon) if yesterday_vals_noon else 0.0 | ||
|
|
||
| # The fixed code restricts rate_low to yesterday's range, so it must equal | ||
| # yesterday's cheap rate (5p) regardless of minutes_now. | ||
| print("rate_low_midnight={}, rate_low_morning={}, rate_low_noon={}".format(rate_low_midnight, rate_low_morning, rate_low_noon)) | ||
|
|
||
| if rate_low_midnight != cheap_rate: | ||
| print("ERROR: rate_low at midnight should be {} (yesterday's min), got {}".format(cheap_rate, rate_low_midnight)) | ||
| failed = 1 | ||
|
|
||
| if rate_low_morning != cheap_rate: | ||
| print("ERROR: rate_low at morning should be {} (yesterday's min), got {}".format(cheap_rate, rate_low_morning)) | ||
| failed = 1 | ||
|
|
||
| if rate_low_noon != cheap_rate: | ||
| print("ERROR: rate_low at noon should be {} (yesterday's min), got {}".format(cheap_rate, rate_low_noon)) | ||
| failed = 1 | ||
|
|
||
| # Demonstrate the OLD (broken) behaviour: using all values instead of yesterday-only | ||
| rate_low_broken_midnight = min(past_rates_midnight.values()) if past_rates_midnight else 0.0 | ||
| rate_low_broken_morning = min(past_rates_morning.values()) if past_rates_morning else 0.0 | ||
|
|
||
| # At midnight the old code would give cheap_rate (yesterday only, 0p not yet included) | ||
| # At morning the old code would give 0p (today's 0p slot entered past_rates) | ||
| print("OLD rate_low_midnight={}, OLD rate_low_morning={}".format(rate_low_broken_midnight, rate_low_broken_morning)) | ||
|
|
||
| if rate_low_broken_midnight != cheap_rate: | ||
| print("INFO: Old midnight rate_low={} (expected {}, this can vary by setup)".format(rate_low_broken_midnight, cheap_rate)) | ||
|
|
||
| if rate_low_broken_morning != today_cheap_rate: | ||
| print("INFO: Old morning rate_low={} (expected {} to demonstrate bug)".format(rate_low_broken_morning, today_cheap_rate)) | ||
| else: | ||
| # The bug is confirmed: old code gives a different (lower) threshold in the morning | ||
| print("Confirmed: old code rate_low drops from {} to {} when today's cheap slot enters past_rates".format(rate_low_broken_midnight, rate_low_broken_morning)) | ||
|
|
||
| # Also verify the variability check (min != max) against yesterday-only values | ||
| # Yesterday has cheap_rate and expensive_rate, so min != max should be True | ||
| no_io_yesterday_vals_noon = [v for k, v in past_rates_noon.items() if k < end_record] | ||
| if not no_io_yesterday_vals_noon: | ||
| print("ERROR: no_io_yesterday_vals should not be empty") | ||
| failed = 1 | ||
| else: | ||
| if min(no_io_yesterday_vals_noon) == max(no_io_yesterday_vals_noon): | ||
| print("ERROR: variability check should be True for yesterday's Agile rates") | ||
| failed = 1 | ||
| else: | ||
| print("OK: yesterday's rates are variable (min={}, max={})".format(min(no_io_yesterday_vals_noon), max(no_io_yesterday_vals_noon))) | ||
|
|
||
| # Ensure charge-window finding (rate_scan_window) uses the correct stable threshold | ||
| # by directly calling it on a past_rates_no_io equivalent using rate_low from the fix | ||
| past_rates_no_io = build_past_rates(720) # noon scenario | ||
| my_predbat.combine_charge_slots = True | ||
| charge_window_best, lowest, highest = my_predbat.rate_scan_window(past_rates_no_io, 5, rate_low_noon, False, return_raw=True) | ||
| # Filter to yesterday's window only (start < end_record) | ||
| charge_window_best = [c for c in charge_window_best if c["start"] < end_record] | ||
| print("Charge windows found with FIXED rate_low={}: {}".format(rate_low_noon, charge_window_best)) | ||
|
|
||
| if not charge_window_best: | ||
| print("ERROR: charge windows should be found with fixed rate_low") | ||
| failed = 1 | ||
| else: | ||
| for cw in charge_window_best: | ||
| if cw["average"] > rate_low_noon + 0.01: | ||
| print("ERROR: charge window average rate {} exceeds threshold {}".format(cw["average"], rate_low_noon)) | ||
| failed = 1 | ||
|
|
||
| # Verify that using the OLD broken rate_low (0p) finds NO windows in yesterday's data | ||
| charge_window_broken, _, _ = my_predbat.rate_scan_window(past_rates_no_io, 5, today_cheap_rate, False, return_raw=True) | ||
| charge_window_broken = [c for c in charge_window_broken if c["start"] < end_record] | ||
| print("Charge windows found with BROKEN rate_low={}: {}".format(today_cheap_rate, charge_window_broken)) | ||
|
|
||
| if charge_window_broken: | ||
| # Only yesterday entries (k < 1440) have non-zero rates, so finding windows at 0p | ||
| # means the scan accidentally hit entries where past_rates_no_io[k] = 0.0 for k<1440 | ||
| # This could be an artifact of how the test builds rate_import. | ||
| # The important thing is that the fixed code does NOT use 0p as the threshold. | ||
| print("INFO: unexpected windows at 0p threshold - may be harmless test setup artifact") | ||
|
springfall2008 marked this conversation as resolved.
Outdated
|
||
|
|
||
|
springfall2008 marked this conversation as resolved.
Outdated
|
||
| return failed | ||
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in da44d39. Extracted the
rate_lowselection into acompute_rate_low_for_yesterday()method on theOutputclass, whichcalculate_yesterday()now calls. The test was rewritten to call the production methods directly:history_to_future_rates()to buildpast_ratesexactly as production does,compute_rate_low_for_yesterday()to assert stability across threeminutes_nowvalues, andrate_scan_window()to confirm correct window detection. All shared state (combine_charge_slots,minutes_now,forecast_minutes) is now saved and restored in atry/finallyblock with deterministic values set beforerate_scan_windowis called.