Skip to content

Commit 20ad859

Browse files
refactor(compare): save and restore a declared set of state, and plan each tariff from scratch
The comparison kept a hand-written list of attributes to save and restore, and plan_preclip leaked because it was missing from it; end_record, the two rate_best_cost thresholds and plan_valid / plan_last_updated were missing too, so after the comparison the next cycle published the last tariff's thresholds and treated the restored plan as freshly computed. run_all() now saves and restores the attributes declared in COMPARE_PLAN_STATE, COMPARE_HARDWARE_STATE (also reset after each tariff) and the rest of COMPARE_RESTORED_STATE, with the car state in COMPARE_RESTORED_COPIES deep-copied as before. Each tariff now plans from scratch (plan_valid False before its run): scored against the previous tariff's plan, the keep-or-switch threshold in calculate_plan() could leave one tariff's result as another tariff's plan. Tests: run_all() with a tariff run that changes every saved attribute must restore all of them, reset the hardware values between tariffs and start each tariff with plan_valid False; and every attribute calculate_plan() assigns must be either in COMPARE_PLAN_STATE or listed as rebuilt every run, so the next new piece of plan state fails a test rather than leaking (checked: dropping end_record from the set fails it). T19 and T25 now check the declared sets rather than the old source text. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 1cd1183 commit 20ad859

2 files changed

Lines changed: 141 additions & 123 deletions

File tree

‎apps/predbat/compare.py‎

Lines changed: 65 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,60 @@
2727
# 3. Consider Octopus API key to access current tariff info and switch links
2828

2929

30+
# What Compare.run_all() saves before comparing tariffs and restores afterwards: every tariff fetches, plans and
31+
# predicts on the live instance. The plan state is what calculate_plan() writes that outlives the run - leaving
32+
# plan_preclip as the last tariff's plan once made the first re-plan after midnight score the wrong plan as the
33+
# plan in force. Add any new plan state calculate_plan() keeps across runs here.
34+
COMPARE_PLAN_STATE = (
35+
"charge_window_best",
36+
"charge_limit_best",
37+
"export_window_best",
38+
"export_limits_best",
39+
"plan_preclip",
40+
"end_record",
41+
"plan_valid",
42+
"plan_last_updated",
43+
"plan_last_updated_minutes",
44+
"rate_best_cost_threshold_charge",
45+
"rate_best_cost_threshold_export",
46+
)
47+
# Battery and inverter values a tariff can override, reset after each tariff as well as at the end
48+
COMPARE_HARDWARE_STATE = ("soc_max", "battery_rate_max_charge", "battery_rate_max_charge_dc", "battery_rate_max_discharge", "battery_rate_max_export", "inverter_limit")
49+
COMPARE_RESTORED_STATE = (
50+
COMPARE_PLAN_STATE
51+
+ COMPARE_HARDWARE_STATE
52+
+ (
53+
"forecast_plan_hours",
54+
"forecast_minutes",
55+
"forecast_days",
56+
"manual_charge_times",
57+
"manual_export_times",
58+
"manual_freeze_charge_times",
59+
"manual_freeze_export_times",
60+
"manual_demand_times",
61+
"manual_all_times",
62+
"cost_today_sofar",
63+
"carbon_today_sofar",
64+
"iboost_today",
65+
"iboost_plan",
66+
"import_today_now",
67+
"export_today_now",
68+
"octopus_intelligent_charging",
69+
"soc_kw",
70+
)
71+
)
72+
# Car state the comparison rebuilds per tariff, deep-copied as the lists are changed in place
73+
COMPARE_RESTORED_COPIES = (
74+
"car_charging_plan_smart",
75+
"car_charging_limit",
76+
"car_charging_limit_model",
77+
"car_charging_soc",
78+
"car_charging_battery_size",
79+
"car_charging_slots",
80+
"car_charging_now_slots",
81+
)
82+
83+
3084
class Compare:
3185
"""Tariff comparison engine.
3286
@@ -605,43 +659,9 @@ def run_all(self, debug=False, fetch_sensor=True):
605659

606660
my_predbat = self.pb
607661

608-
save_forecast_plan_hours = my_predbat.forecast_plan_hours
609-
save_forecast_minutes = my_predbat.forecast_minutes
610-
save_forecast_days = my_predbat.forecast_days
611-
save_manual_charge_times = my_predbat.manual_charge_times
612-
save_manual_export_times = my_predbat.manual_export_times
613-
save_manual_freeze_charge_times = my_predbat.manual_freeze_charge_times
614-
save_manual_freeze_export_times = my_predbat.manual_freeze_export_times
615-
save_manual_demand_times = my_predbat.manual_demand_times
616-
save_manual_all_times = my_predbat.manual_all_times
617-
save_charge_window_best = my_predbat.charge_window_best
618-
save_export_window_best = my_predbat.export_window_best
619-
save_export_limits_best = my_predbat.export_limits_best
620-
save_charge_limit_best = my_predbat.charge_limit_best
621-
# The pre-clip copy of the plan in force, which the next re-plan scores as the previous plan; each tariff's
622-
# calculate_plan() replaces it with its own plan
623-
save_plan_preclip = my_predbat.plan_preclip
624-
save_cost_today_sofar = my_predbat.cost_today_sofar
625-
save_carbon_today_sofar = my_predbat.carbon_today_sofar
626-
save_iboost_today = my_predbat.iboost_today
627-
save_iboost_plan = my_predbat.iboost_plan
628-
save_import_today_now = my_predbat.import_today_now
629-
save_export_today_now = my_predbat.export_today_now
630-
save_octopus_intelligent_charging = my_predbat.octopus_intelligent_charging
631-
save_car_charging_plan_smart = copy.deepcopy(my_predbat.car_charging_plan_smart)
632-
save_car_charging_limit = copy.deepcopy(my_predbat.car_charging_limit)
633-
save_car_charging_limit_model = copy.deepcopy(my_predbat.car_charging_limit_model)
634-
save_car_charging_soc = copy.deepcopy(my_predbat.car_charging_soc)
635-
save_car_charging_battery_size = copy.deepcopy(my_predbat.car_charging_battery_size)
636-
save_car_charging_slots = copy.deepcopy(my_predbat.car_charging_slots)
637-
save_car_charging_now_slots = copy.deepcopy(my_predbat.car_charging_now_slots)
638-
save_soc_kw = my_predbat.soc_kw
639-
save_soc_max = my_predbat.soc_max
640-
save_battery_rate_max_charge = my_predbat.battery_rate_max_charge
641-
save_battery_rate_max_charge_dc = my_predbat.battery_rate_max_charge_dc
642-
save_battery_rate_max_discharge = my_predbat.battery_rate_max_discharge
643-
save_battery_rate_max_export = my_predbat.battery_rate_max_export
644-
save_inverter_limit = my_predbat.inverter_limit
662+
# Everything the comparison changes on the live instance, saved here and put back in the finally below
663+
saved = {name: getattr(my_predbat, name) for name in COMPARE_RESTORED_STATE}
664+
saved.update({name: copy.deepcopy(getattr(my_predbat, name)) for name in COMPARE_RESTORED_COPIES})
645665

646666
# Final reports, cut end_record back to 24 hours to ignore the dump at end of day
647667
end_record = int((my_predbat.minutes_now + 24 * 60 + 29) / 30) * 30 - my_predbat.minutes_now
@@ -688,19 +708,18 @@ def run_all(self, debug=False, fetch_sensor=True):
688708
# First ever run for this tariff: start from actual midnight SoC
689709
start_soc = soc_midnight_fallback
690710
self.log("Compare tariff {} starting SoC: {:.2f} kWh".format(tariff.get("id", ""), start_soc))
711+
# Each tariff plans from scratch: scored against the previous tariff's plan, the keep-or-switch threshold
712+
# in calculate_plan() could leave one tariff's result as another tariff's plan
713+
my_predbat.plan_valid = False
691714
try:
692-
result_data = self.run_single(tariff, rate_import_base, rate_export_base, end_record, debug=debug, fetch_sensor=fetch_sensor, car_charging_slots=save_car_charging_slots, start_soc=start_soc, io_adjusted_base=io_adjusted_base)
715+
result_data = self.run_single(tariff, rate_import_base, rate_export_base, end_record, debug=debug, fetch_sensor=fetch_sensor, car_charging_slots=saved["car_charging_slots"], start_soc=start_soc, io_adjusted_base=io_adjusted_base)
693716
if result_data is not None:
694717
results[tariff["id"]] = result_data
695718
finally:
696719
# Restore hardware settings after each tariff so overrides don't bleed into the next tariff;
697720
# in a finally so a raised run_single() can't leave compare values applied to the live instance
698-
my_predbat.soc_max = save_soc_max
699-
my_predbat.battery_rate_max_charge = save_battery_rate_max_charge
700-
my_predbat.battery_rate_max_charge_dc = save_battery_rate_max_charge_dc
701-
my_predbat.battery_rate_max_discharge = save_battery_rate_max_discharge
702-
my_predbat.battery_rate_max_export = save_battery_rate_max_export
703-
my_predbat.inverter_limit = save_inverter_limit
721+
for name in COMPARE_HARDWARE_STATE:
722+
setattr(my_predbat, name, saved[name])
704723
# Restore config: overrides after each tariff too, for the same reason - otherwise a
705724
# tariff's config override stays applied to config_index for every subsequent tariff,
706725
# even ones with no config block of their own (issue #4156)
@@ -725,39 +744,6 @@ def run_all(self, debug=False, fetch_sensor=True):
725744
my_predbat.fetch_config_options()
726745

727746
# Restore original settings
728-
my_predbat.forecast_plan_hours = save_forecast_plan_hours
729-
my_predbat.forecast_minutes = save_forecast_minutes
730-
my_predbat.forecast_days = save_forecast_days
731-
my_predbat.manual_charge_times = save_manual_charge_times
732-
my_predbat.manual_export_times = save_manual_export_times
733-
my_predbat.manual_freeze_charge_times = save_manual_freeze_charge_times
734-
my_predbat.manual_freeze_export_times = save_manual_freeze_export_times
735-
my_predbat.manual_demand_times = save_manual_demand_times
736-
my_predbat.manual_all_times = save_manual_all_times
737-
my_predbat.charge_window_best = save_charge_window_best
738-
my_predbat.export_window_best = save_export_window_best
739-
my_predbat.export_limits_best = save_export_limits_best
740-
my_predbat.charge_limit_best = save_charge_limit_best
741-
my_predbat.plan_preclip = save_plan_preclip
742-
my_predbat.cost_today_sofar = save_cost_today_sofar
743-
my_predbat.carbon_today_sofar = save_carbon_today_sofar
744-
my_predbat.iboost_today = save_iboost_today
745-
my_predbat.iboost_plan = save_iboost_plan
746-
my_predbat.import_today_now = save_import_today_now
747-
my_predbat.export_today_now = save_export_today_now
748-
my_predbat.octopus_intelligent_charging = save_octopus_intelligent_charging
749-
my_predbat.car_charging_limit = save_car_charging_limit
750-
my_predbat.car_charging_limit_model = save_car_charging_limit_model
751-
my_predbat.car_charging_soc = save_car_charging_soc
752-
my_predbat.car_charging_battery_size = save_car_charging_battery_size
753-
my_predbat.car_charging_slots = save_car_charging_slots
754-
my_predbat.car_charging_now_slots = save_car_charging_now_slots
755-
my_predbat.car_charging_plan_smart = save_car_charging_plan_smart
756-
my_predbat.soc_kw = save_soc_kw
757-
my_predbat.soc_max = save_soc_max
758-
my_predbat.battery_rate_max_charge = save_battery_rate_max_charge
759-
my_predbat.battery_rate_max_charge_dc = save_battery_rate_max_charge_dc
760-
my_predbat.battery_rate_max_discharge = save_battery_rate_max_discharge
761-
my_predbat.battery_rate_max_export = save_battery_rate_max_export
762-
my_predbat.inverter_limit = save_inverter_limit
747+
for name in COMPARE_RESTORED_STATE + COMPARE_RESTORED_COPIES:
748+
setattr(my_predbat, name, saved[name])
763749
my_predbat.io_adjusted = io_adjusted_base

‎apps/predbat/tests/test_compare.py‎

Lines changed: 76 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,14 @@
1616
- hardware isolation between tariffs: overrides applied for tariff N must not
1717
bleed into tariff N+1 in run_all() (regression for the bug where agile_fixed
1818
reported a final SOC larger than the normal battery size)
19+
- run_all() restores every attribute it saves, the plan state calculate_plan() keeps across runs
20+
included (regression for plan_preclip left as the last tariff's plan), and plans each tariff from scratch
21+
- every attribute calculate_plan() assigns is either restored by run_all() or rebuilt on every run
1922
"""
2023

2124
import inspect
2225

23-
from compare import Compare
26+
from compare import Compare, COMPARE_HARDWARE_STATE, COMPARE_PLAN_STATE, COMPARE_RESTORED_STATE, COMPARE_RESTORED_COPIES
2427
from const import MINUTE_WATT
2528

2629

@@ -68,28 +71,45 @@ def _make_compare():
6871
return cmp, pb
6972

7073

71-
def test_run_all_restores_plan_preclip(my_predbat):
72-
"""run_all() puts back the pre-clip copy of the plan in force, which each tariff's calculate_plan() replaces.
73-
74-
Left as the last tariff's 48-hour plan, the first re-plan after the midnight comparison scored that plan as the
75-
previous one, so it always switched plans whatever metric_min_improvement_plan said (seen live on 9 Oct 2026:
76-
previous plan 300.97p against 30.71p when the comparison is not run).
74+
# Assigned by calculate_plan() but rebuilt on every run before use, so the comparison need not restore them
75+
CALCULATE_PLAN_REBUILT = (
76+
"load_minutes_step",
77+
"load_minutes_step10",
78+
"load_minutes_step90",
79+
"pv_forecast_minute_step",
80+
"pv_forecast_minute10_step",
81+
"pv_forecast_minute90_step",
82+
"prediction",
83+
"metric_cloud_coverage",
84+
"metric_load_divergence",
85+
"plan_last_duration_seconds",
86+
)
87+
88+
89+
def test_run_all_restores_state(my_predbat):
90+
"""run_all() puts back everything it saves however a tariff's run changed it, resets the hardware values between
91+
tariffs, and plans each tariff from scratch.
92+
93+
Each tariff fetches, plans and predicts on the live instance. Leaving plan_preclip as the last tariff's 48-hour
94+
plan made the first re-plan after the midnight comparison score that plan as the plan in force, so it always
95+
switched plans whatever metric_min_improvement_plan said (seen live on 9 Oct 2026: 300.97p against 30.71p).
7796
"""
78-
from compare import Compare
79-
97+
names = COMPARE_RESTORED_STATE + COMPARE_RESTORED_COPIES
98+
original = {name: getattr(my_predbat, name) for name in names}
8099
saved_compare_list = my_predbat.args.get("compare_list")
81-
saved_preclip = my_predbat.plan_preclip
82-
plan_in_force = ([18.08], [{"start": 0, "end": 90, "average": 7.62}], [], [])
83-
my_predbat.plan_preclip = plan_in_force
84-
my_predbat.args["compare_list"] = [{"id": "test_tariff", "name": "Test tariff"}]
100+
my_predbat.args["compare_list"] = [{"id": "tariff_a", "name": "Tariff A"}, {"id": "tariff_b", "name": "Tariff B"}]
85101
comparison = Compare.__new__(Compare)
86102
comparison.pb = my_predbat
87103
comparison.log = my_predbat.log
88104
comparison.comparisons = {}
105+
seen_at_start = []
89106

90107
def run_single(*args, **kwargs):
91-
"""Stand in for a tariff's run: its calculate_plan() replaces plan_preclip with that tariff's plan."""
92-
my_predbat.plan_preclip = ([0.0], [{"start": 0, "end": 2880, "average": 30.0}], [], [])
108+
"""Stand in for a tariff's run, changing every saved attribute as fetching and planning a tariff can."""
109+
seen_at_start.append((my_predbat.plan_valid, all(getattr(my_predbat, name) == original[name] for name in COMPARE_HARDWARE_STATE)))
110+
for name in names:
111+
setattr(my_predbat, name, ("changed by", name))
112+
my_predbat.plan_valid = True
93113
return None
94114

95115
comparison.run_single = run_single
@@ -98,24 +118,50 @@ def run_single(*args, **kwargs):
98118
comparison.publish_data = lambda *args, **kwargs: None
99119
try:
100120
comparison.run_all(fetch_sensor=False)
101-
if my_predbat.plan_preclip is not plan_in_force:
102-
print("ERROR: run_all() should restore the plan in force's pre-clip copy, got {}".format(my_predbat.plan_preclip))
103-
return 1
104-
print("PASS: run_all() restores plan_preclip")
105-
return 0
121+
failed = 0
122+
not_restored = [name for name in names if getattr(my_predbat, name) != original[name]]
123+
if not_restored:
124+
print("ERROR: run_all() did not restore {}".format(not_restored))
125+
failed = 1
126+
if seen_at_start != [(False, True), (False, True)]:
127+
print("ERROR: each tariff should start with plan_valid False and the real hardware values, got {}".format(seen_at_start))
128+
failed = 1
129+
if not failed:
130+
print("PASS: run_all() restores all {} saved attributes, resets hardware between tariffs and plans each from scratch".format(len(names)))
131+
return failed
106132
finally:
107-
my_predbat.plan_preclip = saved_preclip
133+
for name, value in original.items():
134+
setattr(my_predbat, name, value)
108135
if saved_compare_list is None:
109136
my_predbat.args.pop("compare_list", None)
110137
else:
111138
my_predbat.args["compare_list"] = saved_compare_list
112139

113140

141+
def test_plan_state_covered(my_predbat):
142+
"""Every attribute calculate_plan() assigns is restored by the comparison or rebuilt on every run.
143+
144+
A new piece of plan state that outlives a run (as plan_preclip did) must be added to COMPARE_PLAN_STATE, or the
145+
comparison will leave the last tariff's value behind.
146+
"""
147+
import re
148+
149+
source = inspect.getsource(type(my_predbat).calculate_plan)
150+
assigned = set(re.findall(r"self\.([a-z_0-9]+)\s*=(?!=)", source))
151+
uncovered = sorted(assigned - set(COMPARE_PLAN_STATE) - set(CALCULATE_PLAN_REBUILT))
152+
if uncovered:
153+
print("ERROR: calculate_plan() assigns {} - add each to COMPARE_PLAN_STATE (compare.py) if it outlives the run, or to CALCULATE_PLAN_REBUILT here if it is rebuilt before use".format(uncovered))
154+
return 1
155+
print("PASS: every attribute calculate_plan() assigns is restored or rebuilt")
156+
return 0
157+
158+
114159
def test_compare(my_predbat):
115160
"""Run all compare unit tests."""
116161
failed = 0
117162
print("**** Running compare tests ****\n")
118-
failed += test_run_all_restores_plan_preclip(my_predbat)
163+
failed += test_run_all_restores_state(my_predbat)
164+
failed += test_plan_state_covered(my_predbat)
119165

120166
# ------------------------------------------------------------------
121167
# T1: apply_hardware_overrides – soc_max override
@@ -588,23 +634,13 @@ def _mock_plan_iboost_smart():
588634
print("PASS T18: bad export override value falls back to the discharge override")
589635

590636
# ------------------------------------------------------------------
591-
# T19: run_all() actually saves and restores battery_rate_max_export,
592-
# both mid-loop and at the end, so an export override can't bleed
593-
# into the next tariff (T7 only proves the pattern in isolation).
594-
# Positional checks (T12-style) so the snapshot must sit before the
595-
# tariff loop, not just anywhere in the source.
637+
# T19: run_all() saves and restores battery_rate_max_export, after each
638+
# tariff and at the end, so an export override can't bleed into the
639+
# next tariff (T7 only proves the pattern in isolation; the restore
640+
# itself is exercised by test_run_all_restores_state)
596641
# ------------------------------------------------------------------
597-
run_all_source = inspect.getsource(Compare.run_all)
598-
loop_idx = run_all_source.find("for tariff in compare_list:")
599-
snapshot_idx = run_all_source.find("save_battery_rate_max_export = my_predbat.battery_rate_max_export")
600-
if loop_idx < 0 or snapshot_idx < 0:
601-
print("ERROR T19: could not locate the tariff loop or the battery_rate_max_export snapshot in run_all() source - test may need updating")
602-
failed += 1
603-
elif not (snapshot_idx < loop_idx):
604-
print("ERROR T19: battery_rate_max_export snapshot must appear before the tariff loop in run_all()")
605-
failed += 1
606-
elif run_all_source.count("my_predbat.battery_rate_max_export = save_battery_rate_max_export") < 2:
607-
print("ERROR T19: run_all() must restore battery_rate_max_export both inside the tariff loop and at the end")
642+
if "battery_rate_max_export" not in COMPARE_HARDWARE_STATE or "battery_rate_max_export" not in COMPARE_RESTORED_STATE:
643+
print("ERROR T19: battery_rate_max_export must be reset after each tariff (COMPARE_HARDWARE_STATE) and at the end")
608644
failed += 1
609645
else:
610646
print("PASS T19: run_all() snapshots and restores battery_rate_max_export")
@@ -708,12 +744,8 @@ def _mock_plan_iboost_smart():
708744
# restores it after it, as it does car_charging_slots, so the live plan
709745
# gets its modelled slots back after a comparison (#5245 review)
710746
# ------------------------------------------------------------------
711-
snapshot_idx = run_all_source.find("save_car_charging_now_slots = copy.deepcopy(my_predbat.car_charging_now_slots)")
712-
if snapshot_idx < 0 or not (snapshot_idx < loop_idx):
713-
print("ERROR T25: car_charging_now_slots must be snapshotted before the tariff loop in run_all()")
714-
failed += 1
715-
elif run_all_source.find("my_predbat.car_charging_now_slots = save_car_charging_now_slots") < loop_idx:
716-
print("ERROR T25: run_all() must restore car_charging_now_slots after the tariff loop")
747+
if "car_charging_now_slots" not in COMPARE_RESTORED_COPIES:
748+
print("ERROR T25: run_all() must snapshot (deep copy) and restore car_charging_now_slots")
717749
failed += 1
718750
else:
719751
print("PASS T25: run_all() snapshots and restores car_charging_now_slots")

0 commit comments

Comments
 (0)