From fac418adbe2de14ad29b40abc74cab73653c4ba3 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Tue, 17 Feb 2026 15:25:51 +0000 Subject: [PATCH 01/46] Don't re-get scenarios for every plan --- .../db/functions/recommendations_functions.py | 7 +++++++ backend/categorisation/processor.py | 21 ++++++++++++------- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/backend/app/db/functions/recommendations_functions.py b/backend/app/db/functions/recommendations_functions.py index e690991a..aa966fbb 100644 --- a/backend/app/db/functions/recommendations_functions.py +++ b/backend/app/db/functions/recommendations_functions.py @@ -625,6 +625,13 @@ def get_plans_by_portfolio_id(portfolio_id: int) -> List[PlanModel]: return session_any.exec(stmt).scalars().all() +def get_scenarios_by_portfolio_id(portfolio_id: int) -> List[ScenarioModel]: + stmt = select(ScenarioModel).where(ScenarioModel.portfolio_id == portfolio_id) + with db_read_session() as session: + session_any: Any = session # Typehint as Any to satisfy Pylance... + return session_any.exec(stmt).scalars().all() + + def get_scenario(scenario_id: int) -> Optional[ScenarioModel]: stmt = select(ScenarioModel).where(ScenarioModel.id == scenario_id) with db_read_session() as session: diff --git a/backend/categorisation/processor.py b/backend/categorisation/processor.py index 7c5698b7..d2bdbef0 100644 --- a/backend/categorisation/processor.py +++ b/backend/categorisation/processor.py @@ -4,7 +4,7 @@ from typing import Dict, List from backend.app.db.functions.recommendations_functions import ( bulk_update_plans, get_plans_by_portfolio_id, - get_scenario, + get_scenarios_by_portfolio_id, ) from backend.app.db.models.recommendations import PlanModel, ScenarioModel from backend.app.domain.classes.plan import Plan @@ -15,7 +15,7 @@ logger = setup_logger() def process_portfolio(portfolio_id: int) -> None: - print(f"Processing portfolio {portfolio_id}") + logger.info(f"Processing portfolio {portfolio_id}") plans: List[Plan] = _load_plans_for_portfolio(portfolio_id) plans_by_property: Dict[int, List[Plan]] = _group_plans_by_property(plans) @@ -29,22 +29,27 @@ def process_portfolio(portfolio_id: int) -> None: def _load_plans_for_portfolio(portfolio_id: int) -> List[Plan]: - plan_models = get_plans_by_portfolio_id(portfolio_id) - print(f"Got {len(plan_models)} plans from database") - plans: List[Plan] = [] + plan_models = get_plans_by_portfolio_id(portfolio_id) + scenarios: List[ScenarioModel] = get_scenarios_by_portfolio_id(portfolio_id) + + if not scenarios: + raise Exception(f"No scenarios found for Portfolio {portfolio_id}") + for model in plan_models: - if not model.scenario_id: + + scenario_model = next((s for s in scenarios if s.id == model.scenario_id)) + if not scenario_model: logger.info(f"No Scenario associated with Plan of ID {model.id}") continue - scenario_model = get_scenario(model.scenario_id) plans.append( Plan.from_sqlalchemy(model, Scenario.from_sqlalchemy(scenario_model)) ) - print("Successfully mapped plan and scenario to domain object") + logger.info("Successfully mapped plan and scenario to domain object") + logger.info(f"Got {len(plans)} plans from database") return plans From 3a5df1a1f3f5e15c9a6bd353d8f957a7d3512e6d Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 18 Feb 2026 09:05:41 +0000 Subject: [PATCH 02/46] Better logging --- backend/categorisation/processor.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/backend/categorisation/processor.py b/backend/categorisation/processor.py index d2bdbef0..97e4c5ad 100644 --- a/backend/categorisation/processor.py +++ b/backend/categorisation/processor.py @@ -47,9 +47,11 @@ def _load_plans_for_portfolio(portfolio_id: int) -> List[Plan]: plans.append( Plan.from_sqlalchemy(model, Scenario.from_sqlalchemy(scenario_model)) ) - logger.info("Successfully mapped plan and scenario to domain object") + logger.debug( + f"Successfully mapped plan {model.id} and scenario {scenario_model.id} to domain object" + ) - logger.info(f"Got {len(plans)} plans from database") + logger.debug(f"Got {len(plans)} plans from database") return plans @@ -83,6 +85,9 @@ def _update_default_flags(plans: List[Plan], cheapest_plan: Plan) -> None: for plan in plans: should_be_default: bool = plan.id == cheapest_plan.id if plan.record.is_default != should_be_default: + logger.info( + f"Setting Plan {plan.id} (Scenario Name: {plan.scenario.record.name}) to is_default: {should_be_default}" + ) plan.set_default(should_be_default) plans_to_update.append(plan) @@ -96,3 +101,7 @@ def _update_default_flags(plans: List[Plan], cheapest_plan: Plan) -> None: scenario_models.append(scenario_model) bulk_update_plans(plan_models, scenario_models) + logger.info("Successfully updated Plan default values") + + else: + logger.info("All plan default values already correct. Not udpating") From 9a177065b611c0fded57070554701f010649b852 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 18 Feb 2026 10:06:40 +0000 Subject: [PATCH 03/46] =?UTF-8?q?allow=20plan=20priority=20to=20be=20speci?= =?UTF-8?q?fied=20for=20plans=20with=20identical=20ouput=20=F0=9F=9F=A5?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../categorisation_trigger_request.py | 3 + backend/categorisation/processor.py | 14 ++- .../tests/test_prioritised_plan_selected.py | 88 +++++++++++++++++++ 3 files changed, 101 insertions(+), 4 deletions(-) create mode 100644 backend/categorisation/tests/test_prioritised_plan_selected.py diff --git a/backend/categorisation/categorisation_trigger_request.py b/backend/categorisation/categorisation_trigger_request.py index 9ef1d106..aa2b8ed3 100644 --- a/backend/categorisation/categorisation_trigger_request.py +++ b/backend/categorisation/categorisation_trigger_request.py @@ -1,5 +1,8 @@ +from typing import List, Optional from pydantic import BaseModel class CategorisationTriggerRequest(BaseModel): portfolio_id: int + + plan_priority_order: Optional[List[int]] diff --git a/backend/categorisation/processor.py b/backend/categorisation/processor.py index 97e4c5ad..539f7a68 100644 --- a/backend/categorisation/processor.py +++ b/backend/categorisation/processor.py @@ -1,5 +1,5 @@ from collections import defaultdict -from typing import Dict, List +from typing import Dict, List, Optional from backend.app.db.functions.recommendations_functions import ( bulk_update_plans, @@ -14,7 +14,9 @@ from utils.logger import setup_logger logger = setup_logger() -def process_portfolio(portfolio_id: int) -> None: +def process_portfolio( + portfolio_id: int, plan_priority_order: Optional[List[int]] = [] +) -> None: logger.info(f"Processing portfolio {portfolio_id}") plans: List[Plan] = _load_plans_for_portfolio(portfolio_id) plans_by_property: Dict[int, List[Plan]] = _group_plans_by_property(plans) @@ -24,7 +26,9 @@ def process_portfolio(portfolio_id: int) -> None: if not property_plans: raise ValueError(f"No plans for property {uprn}") - cheapest_plan = _choose_cheapest_relevant_plan(property_plans) + cheapest_plan = _choose_cheapest_relevant_plan( + property_plans, plan_priority_order + ) _update_default_flags(property_plans, cheapest_plan) @@ -64,7 +68,9 @@ def _group_plans_by_property(plans: List[Plan]) -> Dict[int, List[Plan]]: return grouped -def _choose_cheapest_relevant_plan(plans: List[Plan]) -> Plan: +def _choose_cheapest_relevant_plan( + plans: List[Plan], plan_priority_order: Optional[List[int]] = [] +) -> Plan: plans_to_consider: List[Plan] = [p for p in plans if p.is_compliant] or plans def plan_cost(plan: Plan) -> float: diff --git a/backend/categorisation/tests/test_prioritised_plan_selected.py b/backend/categorisation/tests/test_prioritised_plan_selected.py new file mode 100644 index 00000000..03bca666 --- /dev/null +++ b/backend/categorisation/tests/test_prioritised_plan_selected.py @@ -0,0 +1,88 @@ +from datetime import datetime +from typing import List +import pytest + +from backend.app.domain.classes.plan import Plan +from backend.app.domain.classes.scenario import Scenario +from backend.app.domain.records.plan_record import PlanRecord +from backend.app.domain.records.scenario_record import ScenarioRecord +from backend.app.db.models.portfolio import Epc, PortfolioGoal +from backend.categorisation.processor import _choose_cheapest_relevant_plan + + +@pytest.fixture +def created_at_datetime() -> datetime: + return datetime.now() + + +@pytest.fixture +def identical_plan_record(created_at_datetime: datetime, default: bool) -> PlanRecord: + return PlanRecord( + property_id=1, + portfolio_id=1, + created_at=created_at_datetime, + is_default=default, + post_epc_rating=Epc.C, + cost_of_works=500.0, + ) + + +def make_plan_record(created_at_datetime: datetime, default: bool) -> PlanRecord: + return PlanRecord( + property_id=1, + portfolio_id=1, + created_at=created_at_datetime, + is_default=default, + post_epc_rating=Epc.C, + cost_of_works=500.0, + ) + + +def test_prioritised_plan_selected(created_at_datetime: datetime) -> None: + # arrange + epc_c_scenario_record = ScenarioRecord( + name="EPC C", + created_at=created_at_datetime, + housing_type="", + goal=PortfolioGoal.INCREASING_EPC, + goal_value="C", + trigger_file_path="", + multi_plan=False, + is_default=True, + ) + epc_c_scenario = Scenario(record=epc_c_scenario_record, id=1) + epc_c_plan = Plan( + record=make_plan_record(created_at_datetime, True), + scenario=epc_c_scenario, + id=1, + ) + + minor_works_scenario_record = ScenarioRecord( + name="EPC C - Minor Works", + created_at=created_at_datetime, + housing_type="", + goal=PortfolioGoal.INCREASING_EPC, + goal_value="C", + trigger_file_path="", + multi_plan=False, + is_default=False, + ) + minor_works_scenario = Scenario(record=minor_works_scenario_record, id=2) + minor_works_plan = Plan( + record=make_plan_record(created_at_datetime, False), + scenario=minor_works_scenario, + id=2, + ) + + plan_priority_order: List[int] = [2, 1] + + expected_default_plan_id = 2 + + # act + actual_default_plan = _choose_cheapest_relevant_plan( + plans=[epc_c_plan, minor_works_plan], + plan_priority_order=plan_priority_order, + ) + + # assert + assert actual_default_plan.id == expected_default_plan_id From 508f3f285934908f5ebeab2b9437dac2c108184e Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 18 Feb 2026 10:40:50 +0000 Subject: [PATCH 04/46] make choose cheapest relevant plan method public as it's called from outside the module --- backend/categorisation/processor.py | 36 +++++++++---------- .../tests/test_prioritised_plan_selected.py | 4 +-- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/backend/categorisation/processor.py b/backend/categorisation/processor.py index 539f7a68..02116d61 100644 --- a/backend/categorisation/processor.py +++ b/backend/categorisation/processor.py @@ -26,12 +26,29 @@ def process_portfolio( if not property_plans: raise ValueError(f"No plans for property {uprn}") - cheapest_plan = _choose_cheapest_relevant_plan( + cheapest_plan = choose_cheapest_relevant_plan( property_plans, plan_priority_order ) _update_default_flags(property_plans, cheapest_plan) +def choose_cheapest_relevant_plan( + plans: List[Plan], plan_priority_order: Optional[List[int]] = [] +) -> Plan: + plans_to_consider: List[Plan] = [p for p in plans if p.is_compliant] or plans + + def plan_cost(plan: Plan) -> float: + return ( + plan.record.cost_of_works + if plan.record.cost_of_works is not None + else float("inf") + ) + + cheapest_plan = min(plans_to_consider, key=plan_cost) + + return cheapest_plan + + def _load_plans_for_portfolio(portfolio_id: int) -> List[Plan]: plans: List[Plan] = [] @@ -68,23 +85,6 @@ def _group_plans_by_property(plans: List[Plan]) -> Dict[int, List[Plan]]: return grouped -def _choose_cheapest_relevant_plan( - plans: List[Plan], plan_priority_order: Optional[List[int]] = [] -) -> Plan: - plans_to_consider: List[Plan] = [p for p in plans if p.is_compliant] or plans - - def plan_cost(plan: Plan) -> float: - return ( - plan.record.cost_of_works - if plan.record.cost_of_works is not None - else float("inf") - ) - - cheapest_plan = min(plans_to_consider, key=plan_cost) - - return cheapest_plan - - def _update_default_flags(plans: List[Plan], cheapest_plan: Plan) -> None: plans_to_update: List[Plan] = [] diff --git a/backend/categorisation/tests/test_prioritised_plan_selected.py b/backend/categorisation/tests/test_prioritised_plan_selected.py index 03bca666..eb41194c 100644 --- a/backend/categorisation/tests/test_prioritised_plan_selected.py +++ b/backend/categorisation/tests/test_prioritised_plan_selected.py @@ -7,7 +7,7 @@ from backend.app.domain.classes.scenario import Scenario from backend.app.domain.records.plan_record import PlanRecord from backend.app.domain.records.scenario_record import ScenarioRecord from backend.app.db.models.portfolio import Epc, PortfolioGoal -from backend.categorisation.processor import _choose_cheapest_relevant_plan +from backend.categorisation.processor import choose_cheapest_relevant_plan @pytest.fixture @@ -79,7 +79,7 @@ def test_prioritised_plan_selected(created_at_datetime: datetime) -> None: expected_default_plan_id = 2 # act - actual_default_plan = _choose_cheapest_relevant_plan( + actual_default_plan = choose_cheapest_relevant_plan( plans=[epc_c_plan, minor_works_plan], plan_priority_order=plan_priority_order, ) From b916551921f3e5590fd1d7caf7270370348361de Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 18 Feb 2026 11:20:08 +0000 Subject: [PATCH 05/46] =?UTF-8?q?allow=20plan=20priority=20to=20be=20speci?= =?UTF-8?q?fied=20for=20plans=20with=20identical=20ouput=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- backend/categorisation/processor.py | 37 ++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/backend/categorisation/processor.py b/backend/categorisation/processor.py index 02116d61..184ccac2 100644 --- a/backend/categorisation/processor.py +++ b/backend/categorisation/processor.py @@ -1,6 +1,8 @@ from collections import defaultdict from typing import Dict, List, Optional +from sqlalchemy import Tuple + from backend.app.db.functions.recommendations_functions import ( bulk_update_plans, get_plans_by_portfolio_id, @@ -33,20 +35,43 @@ def process_portfolio( def choose_cheapest_relevant_plan( - plans: List[Plan], plan_priority_order: Optional[List[int]] = [] + plans: List[Plan], plan_priority_order: Optional[List[int]] = None ) -> Plan: - plans_to_consider: List[Plan] = [p for p in plans if p.is_compliant] or plans + plan_priority_order = plan_priority_order or [] - def plan_cost(plan: Plan) -> float: - return ( + eligible_plans: List[Plan] = [plan for plan in plans if plan.is_compliant] or plans + if not eligible_plans: + raise ValueError("No plans available to choose from.") + + for plan in eligible_plans: + if plan.id is None: + # This should never actually happen, but plan.id is optional to cater + # for new plans. We are only working with already persisted plans here + raise ValueError( + f"All plans must have an ID, but found a plan with no ID: {plan}" + ) + + min_cost: float = min( + ( plan.record.cost_of_works if plan.record.cost_of_works is not None else float("inf") ) + for plan in eligible_plans + ) - cheapest_plan = min(plans_to_consider, key=plan_cost) + cheapest_plans: List[Plan] = [ + plan + for plan in eligible_plans + if (plan.record.cost_of_works or float("inf")) == min_cost + ] - return cheapest_plan + for priority_plan_id in plan_priority_order: + for plan in cheapest_plans: + if plan.id == priority_plan_id: + return plan + + return cheapest_plans[0] def _load_plans_for_portfolio(portfolio_id: int) -> List[Plan]: From bfb0d79da6c7d7e0f8e9465683c14e7141e0b1ea Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 18 Feb 2026 11:27:09 +0000 Subject: [PATCH 06/46] =?UTF-8?q?Cheapest=20compliant=20plan=20selected=20?= =?UTF-8?q?even=20when=20not=20in=20the=20priority=20list=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../tests/test_prioritised_plan_selected.py | 100 ++++++++++-------- 1 file changed, 53 insertions(+), 47 deletions(-) diff --git a/backend/categorisation/tests/test_prioritised_plan_selected.py b/backend/categorisation/tests/test_prioritised_plan_selected.py index eb41194c..5424dd5e 100644 --- a/backend/categorisation/tests/test_prioritised_plan_selected.py +++ b/backend/categorisation/tests/test_prioritised_plan_selected.py @@ -15,67 +15,73 @@ def created_at_datetime() -> datetime: return datetime.now() -@pytest.fixture -def identical_plan_record(created_at_datetime: datetime, default: bool) -> PlanRecord: +def make_plan_record( + created_at: datetime, default: bool, cost_of_works: float = 500.0 +) -> PlanRecord: return PlanRecord( property_id=1, portfolio_id=1, - created_at=created_at_datetime, + created_at=created_at, is_default=default, post_epc_rating=Epc.C, - cost_of_works=500.0, + cost_of_works=cost_of_works, ) -def make_plan_record(created_at_datetime: datetime, default: bool) -> PlanRecord: - return PlanRecord( - property_id=1, - portfolio_id=1, - created_at=created_at_datetime, - is_default=default, - post_epc_rating=Epc.C, - cost_of_works=500.0, +def make_scenario(name: str, created_at: datetime, is_default: bool) -> Scenario: + record = ScenarioRecord( + name=name, + created_at=created_at, + housing_type="", + goal=PortfolioGoal.INCREASING_EPC, + goal_value="C", + trigger_file_path="", + multi_plan=False, + is_default=is_default, + ) + return Scenario(record=record, id=1 if is_default else 2) + + +def make_plan( + created_at: datetime, default: bool, cost_of_works: float = 500.0, name: str = "" +) -> Plan: + scenario = make_scenario(name, created_at, default) + plan_id = 1 if default else 2 + return Plan( + record=make_plan_record(created_at, default, cost_of_works), + scenario=scenario, + id=plan_id, ) def test_prioritised_plan_selected(created_at_datetime: datetime) -> None: # arrange - epc_c_scenario_record = ScenarioRecord( - name="EPC C", - created_at=created_at_datetime, - housing_type="", - goal=PortfolioGoal.INCREASING_EPC, - goal_value="C", - trigger_file_path="", - multi_plan=False, - is_default=True, - ) - epc_c_scenario = Scenario(record=epc_c_scenario_record, id=1) - epc_c_plan = Plan( - record=make_plan_record(created_at_datetime, True), - scenario=epc_c_scenario, - id=1, - ) - - minor_works_scenario_record = ScenarioRecord( - name="EPC C - Minor Works", - created_at=created_at_datetime, - housing_type="", - goal=PortfolioGoal.INCREASING_EPC, - goal_value="C", - trigger_file_path="", - multi_plan=False, - is_default=False, - ) - minor_works_scenario = Scenario(record=minor_works_scenario_record, id=2) - minor_works_plan = Plan( - record=make_plan_record(created_at_datetime, False), - scenario=minor_works_scenario, - id=2, - ) - + epc_c_plan = make_plan(created_at_datetime, True, name="EPC C") + minor_works_plan = make_plan(created_at_datetime, False, name="EPC C - Minor Works") plan_priority_order: List[int] = [2, 1] - + expected_default_plan_id = 2 + + # act + actual_default_plan = choose_cheapest_relevant_plan( + plans=[epc_c_plan, minor_works_plan], + plan_priority_order=plan_priority_order, + ) + + # assert + assert actual_default_plan.id == expected_default_plan_id + + +def test_cheapest_plan_returned_if_not_in_priority_list( + created_at_datetime: datetime, +) -> None: + # arrange + epc_c_plan = make_plan( + created_at_datetime, True, cost_of_works=1000.0, name="EPC C" + ) + minor_works_plan = make_plan( + created_at_datetime, False, cost_of_works=100.0, name="EPC C - Minor Works" + ) + plan_priority_order: List[int] = [1, 3] expected_default_plan_id = 2 # act From cc901d999b538e1743a32a00ab474460f3fd7bc5 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 18 Feb 2026 11:54:55 +0000 Subject: [PATCH 07/46] option to only consider a specific list of plans --- .../db/functions/recommendations_functions.py | 7 +++++ .../categorisation_trigger_request.py | 3 +- backend/categorisation/handler/handler.py | 6 +++- backend/categorisation/processor.py | 30 +++++++++++++++---- 4 files changed, 38 insertions(+), 8 deletions(-) diff --git a/backend/app/db/functions/recommendations_functions.py b/backend/app/db/functions/recommendations_functions.py index aa966fbb..d4c3fcb9 100644 --- a/backend/app/db/functions/recommendations_functions.py +++ b/backend/app/db/functions/recommendations_functions.py @@ -625,6 +625,13 @@ def get_plans_by_portfolio_id(portfolio_id: int) -> List[PlanModel]: return session_any.exec(stmt).scalars().all() +def get_plans_by_ids(ids: List[int]) -> List[PlanModel]: + stmt = select(PlanModel).where(PlanModel.id.in_(ids)) + with db_read_session() as session: + session_any: Any = session # Typehint as Any to satisfy Pylance... + return session_any.exec(stmt).scalars().all() + + def get_scenarios_by_portfolio_id(portfolio_id: int) -> List[ScenarioModel]: stmt = select(ScenarioModel).where(ScenarioModel.portfolio_id == portfolio_id) with db_read_session() as session: diff --git a/backend/categorisation/categorisation_trigger_request.py b/backend/categorisation/categorisation_trigger_request.py index aa2b8ed3..46ce6f1c 100644 --- a/backend/categorisation/categorisation_trigger_request.py +++ b/backend/categorisation/categorisation_trigger_request.py @@ -5,4 +5,5 @@ from pydantic import BaseModel class CategorisationTriggerRequest(BaseModel): portfolio_id: int - plan_priority_order: Optional[List[int]] + plans_to_consider: Optional[List[int]] = None + plan_priority_order: Optional[List[int]] = None diff --git a/backend/categorisation/handler/handler.py b/backend/categorisation/handler/handler.py index 20076613..449c5ccf 100644 --- a/backend/categorisation/handler/handler.py +++ b/backend/categorisation/handler/handler.py @@ -20,7 +20,11 @@ def handler(event: Mapping[str, Any], context: Any) -> None: logger.debug("Successfully validated request body") - process_portfolio(payload.portfolio_id) + process_portfolio( + payload.portfolio_id, + payload.plans_to_consider, + payload.plan_priority_order, + ) except Exception as e: logger.error(f"Failed to process record: {e}") diff --git a/backend/categorisation/processor.py b/backend/categorisation/processor.py index 184ccac2..b7ddfc62 100644 --- a/backend/categorisation/processor.py +++ b/backend/categorisation/processor.py @@ -1,10 +1,9 @@ from collections import defaultdict from typing import Dict, List, Optional -from sqlalchemy import Tuple - from backend.app.db.functions.recommendations_functions import ( bulk_update_plans, + get_plans_by_ids, get_plans_by_portfolio_id, get_scenarios_by_portfolio_id, ) @@ -17,10 +16,14 @@ logger = setup_logger() def process_portfolio( - portfolio_id: int, plan_priority_order: Optional[List[int]] = [] + portfolio_id: int, + plans_to_consider: Optional[List[int]] = None, + plan_priority_order: Optional[List[int]] = None, ) -> None: logger.info(f"Processing portfolio {portfolio_id}") - plans: List[Plan] = _load_plans_for_portfolio(portfolio_id) + + plans: List[Plan] = _load_plans_for_portfolio(portfolio_id, plans_to_consider) + plans_by_property: Dict[int, List[Plan]] = _group_plans_by_property(plans) for uprn, property_plans in plans_by_property.items(): @@ -74,10 +77,25 @@ def choose_cheapest_relevant_plan( return cheapest_plans[0] -def _load_plans_for_portfolio(portfolio_id: int) -> List[Plan]: +def _load_plans_for_portfolio( + portfolio_id: int, plans_to_consider: Optional[List[int]] = None +) -> List[Plan]: + + if plans_to_consider: + if len(plans_to_consider) < 2: + raise ValueError("Cannot run auto categorisation for fewer than 2 plans") + + logger.info(f"Getting {len(plans_to_consider)} Plans") + plan_models: List[PlanModel] = get_plans_by_ids(plans_to_consider) + + else: + logger.info( + f"No list of Plans to consider provided. Getting all Plans for portfolio {portfolio_id}" + ) + plan_models: List[PlanModel] = get_plans_by_portfolio_id(portfolio_id) + plans: List[Plan] = [] - plan_models = get_plans_by_portfolio_id(portfolio_id) scenarios: List[ScenarioModel] = get_scenarios_by_portfolio_id(portfolio_id) if not scenarios: From 3dbe118b38064b6e93884b2e80ba6a92ab082365 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 18 Feb 2026 13:29:16 +0000 Subject: [PATCH 08/46] additional logs in handler for local testing --- backend/categorisation/handler/handler.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/backend/categorisation/handler/handler.py b/backend/categorisation/handler/handler.py index 449c5ccf..ee0e7a7d 100644 --- a/backend/categorisation/handler/handler.py +++ b/backend/categorisation/handler/handler.py @@ -1,5 +1,6 @@ import json from typing import Any, Mapping + from backend.categorisation.categorisation_trigger_request import ( CategorisationTriggerRequest, ) @@ -12,6 +13,10 @@ logger = setup_logger() def handler(event: Mapping[str, Any], context: Any) -> None: + logger.info("Received message") + + logger.info(f"Number of events: {len(event.get('Records', []))}") + for record in event.get("Records", []): try: body_dict = json.loads(record["body"]) @@ -27,4 +32,5 @@ def handler(event: Mapping[str, Any], context: Any) -> None: ) except Exception as e: + logger.info("Handler exception") logger.error(f"Failed to process record: {e}") From f7fe7132c7a1f2a86eed8b239070584723ce0a88 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 18 Feb 2026 15:12:24 +0000 Subject: [PATCH 09/46] docker compose for running lambdas locally --- backend/docker-compose-local-lambdas.yml | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 backend/docker-compose-local-lambdas.yml diff --git a/backend/docker-compose-local-lambdas.yml b/backend/docker-compose-local-lambdas.yml new file mode 100644 index 00000000..781f4955 --- /dev/null +++ b/backend/docker-compose-local-lambdas.yml @@ -0,0 +1,11 @@ +version: "3.9" + +services: + lambda: + build: + context: ../ + dockerfile: backend/categorisation/handler/Dockerfile + ports: + - "9000:8080" + env_file: + - ../.env \ No newline at end of file From 3f022ba5488499fe630739c52a71cb5a572d908d Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 18 Feb 2026 15:13:24 +0000 Subject: [PATCH 10/46] rename service in docker compose --- backend/docker-compose-local-lambdas.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/docker-compose-local-lambdas.yml b/backend/docker-compose-local-lambdas.yml index 781f4955..50e9193b 100644 --- a/backend/docker-compose-local-lambdas.yml +++ b/backend/docker-compose-local-lambdas.yml @@ -1,7 +1,7 @@ version: "3.9" services: - lambda: + categorisation-lambda: build: context: ../ dockerfile: backend/categorisation/handler/Dockerfile From b4e3dc9f42a8d24674ef19a72f8efbe75e178610 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 18 Feb 2026 15:19:36 +0000 Subject: [PATCH 11/46] add example request body --- backend/categorisation/categorisation_trigger_request.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/backend/categorisation/categorisation_trigger_request.py b/backend/categorisation/categorisation_trigger_request.py index 46ce6f1c..9bd7d7c8 100644 --- a/backend/categorisation/categorisation_trigger_request.py +++ b/backend/categorisation/categorisation_trigger_request.py @@ -7,3 +7,6 @@ class CategorisationTriggerRequest(BaseModel): plans_to_consider: Optional[List[int]] = None plan_priority_order: Optional[List[int]] = None + + +# {"portfolio_id": 556, "plans_to_consider": [1589319,1589320], "plan_priority_order": [1589319,1589320]} From 478947b8da91320f55b5e452e9803c90dec7cc6d Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 18 Feb 2026 16:12:23 +0000 Subject: [PATCH 12/46] move docker compose back to categorisation directory and define simple invoke script --- .../local_handler/docker-compose.yml | 11 ++++++++ .../local_handler/invoke_local_lambda.py | 25 +++++++++++++++++++ 2 files changed, 36 insertions(+) create mode 100644 backend/categorisation/local_handler/docker-compose.yml create mode 100644 backend/categorisation/local_handler/invoke_local_lambda.py diff --git a/backend/categorisation/local_handler/docker-compose.yml b/backend/categorisation/local_handler/docker-compose.yml new file mode 100644 index 00000000..9529fdb2 --- /dev/null +++ b/backend/categorisation/local_handler/docker-compose.yml @@ -0,0 +1,11 @@ +version: "3.9" + +services: + categorisation-lambda: + build: + context: ../../../ + dockerfile: backend/categorisation/handler/Dockerfile + ports: + - "9000:8080" + env_file: + - ../../../.env \ No newline at end of file diff --git a/backend/categorisation/local_handler/invoke_local_lambda.py b/backend/categorisation/local_handler/invoke_local_lambda.py new file mode 100644 index 00000000..9eb5adda --- /dev/null +++ b/backend/categorisation/local_handler/invoke_local_lambda.py @@ -0,0 +1,25 @@ +#!/usr/bin/env python3 +import json +import requests + +LAMBDA_URL = "http://localhost:9000/2015-03-31/functions/function/invocations" + +payload = { + "Records": [ + { + "body": json.dumps( + { + "portfolio_id": 556, + "plans_to_consider": [], + "plan_priority_order": [], + } + ) + } + ] +} + +response = requests.post(LAMBDA_URL, json=payload) + +print("Status code:", response.status_code) +print("Response:") +print(response.text) From 490c8946d721725b16a90d40903ff667757b86cb Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 18 Feb 2026 17:11:00 +0000 Subject: [PATCH 13/46] Unset existing default before setting new one --- .../db/functions/recommendations_functions.py | 34 +++++++++++++++++++ .../local_handler/invoke_local_lambda.py | 2 +- backend/categorisation/processor.py | 16 +++++++-- 3 files changed, 48 insertions(+), 4 deletions(-) diff --git a/backend/app/db/functions/recommendations_functions.py b/backend/app/db/functions/recommendations_functions.py index d4c3fcb9..3af9fd29 100644 --- a/backend/app/db/functions/recommendations_functions.py +++ b/backend/app/db/functions/recommendations_functions.py @@ -646,6 +646,40 @@ def get_scenario(scenario_id: int) -> Optional[ScenarioModel]: return session_any.exec(stmt).scalar_one_or_none() +def get_default_plan_ids_for_property(property_id: int) -> List[int]: + # This should in reality always return exactly 1 ID, but there's currently + # no database constraint to enforce that, so account for 0 or >1 + stmt = select(PlanModel.id).where( + PlanModel.property_id == property_id and PlanModel.is_default + ) + with db_read_session() as session: + session_any: Any = session # Typehint as Any to satisfy Pylance... + return session_any.exec(stmt).scalars().all() + + +def set_plan_and_scenario_default(plan_id: int, default: bool) -> bool: + with db_session() as session: + plan: PlanModel = session.get(PlanModel, plan_id) + if not plan: + return False + + scenario_id = plan.scenario_id + + plan_mapper: Mapper[Any] = inspect(PlanModel) + scenario_mapper: Mapper[Any] = inspect(ScenarioModel) + + plan_mappings: List[Dict[str, Any]] = [{"id": plan.id, "is_default": default}] + scenario_mappings: List[Dict[str, Any]] = [ + {"id": scenario_id, "is_default": default} + ] + + session.bulk_update_mappings(plan_mapper, plan_mappings) + session.bulk_update_mappings(scenario_mapper, scenario_mappings) + session.commit() + + return True + + def bulk_update_plans( plan_models: List[PlanModel], scenario_models: List[ScenarioModel], diff --git a/backend/categorisation/local_handler/invoke_local_lambda.py b/backend/categorisation/local_handler/invoke_local_lambda.py index 9eb5adda..23e5fda2 100644 --- a/backend/categorisation/local_handler/invoke_local_lambda.py +++ b/backend/categorisation/local_handler/invoke_local_lambda.py @@ -10,7 +10,7 @@ payload = { "body": json.dumps( { "portfolio_id": 556, - "plans_to_consider": [], + "plans_to_consider": [1589319, 1589320], "plan_priority_order": [], } ) diff --git a/backend/categorisation/processor.py b/backend/categorisation/processor.py index b7ddfc62..b07f1c3b 100644 --- a/backend/categorisation/processor.py +++ b/backend/categorisation/processor.py @@ -3,9 +3,11 @@ from typing import Dict, List, Optional from backend.app.db.functions.recommendations_functions import ( bulk_update_plans, + get_default_plan_ids_for_property, get_plans_by_ids, get_plans_by_portfolio_id, get_scenarios_by_portfolio_id, + set_plan_and_scenario_default, ) from backend.app.db.models.recommendations import PlanModel, ScenarioModel from backend.app.domain.classes.plan import Plan @@ -26,15 +28,23 @@ def process_portfolio( plans_by_property: Dict[int, List[Plan]] = _group_plans_by_property(plans) - for uprn, property_plans in plans_by_property.items(): + for property_id, property_plans in plans_by_property.items(): if not property_plans: - raise ValueError(f"No plans for property {uprn}") + raise ValueError(f"No plans for property {property_id}") cheapest_plan = choose_cheapest_relevant_plan( property_plans, plan_priority_order ) - _update_default_flags(property_plans, cheapest_plan) + + # Unset existing default(s) in case they are outside the plans to consider + default_plan_ids: List[int] = get_default_plan_ids_for_property(property_id) + for id in default_plan_ids: + set_plan_and_scenario_default(id, False) + + _update_default_flags( + property_plans, cheapest_plan + ) # TODO: we have already unset existing default(s), so this method can probably be a bit simpler now def choose_cheapest_relevant_plan( From 475b3d0e1305a801857bf13dde0915d80482894e Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 18 Feb 2026 17:19:38 +0000 Subject: [PATCH 14/46] input is list of scenarios to consider not list of plans --- .../db/functions/recommendations_functions.py | 7 +++++++ .../categorisation_trigger_request.py | 2 +- backend/categorisation/handler/handler.py | 2 +- .../local_handler/invoke_local_lambda.py | 2 +- backend/categorisation/processor.py | 19 +++++++++++-------- 5 files changed, 21 insertions(+), 11 deletions(-) diff --git a/backend/app/db/functions/recommendations_functions.py b/backend/app/db/functions/recommendations_functions.py index 3af9fd29..6f7dd41f 100644 --- a/backend/app/db/functions/recommendations_functions.py +++ b/backend/app/db/functions/recommendations_functions.py @@ -632,6 +632,13 @@ def get_plans_by_ids(ids: List[int]) -> List[PlanModel]: return session_any.exec(stmt).scalars().all() +def get_plans_by_scenario_ids(ids: List[int]) -> List[PlanModel]: + stmt = select(PlanModel).where(PlanModel.scenario_id.in_(ids)) + with db_read_session() as session: + session_any: Any = session # Typehint as Any to satisfy Pylance... + return session_any.exec(stmt).scalars().all() + + def get_scenarios_by_portfolio_id(portfolio_id: int) -> List[ScenarioModel]: stmt = select(ScenarioModel).where(ScenarioModel.portfolio_id == portfolio_id) with db_read_session() as session: diff --git a/backend/categorisation/categorisation_trigger_request.py b/backend/categorisation/categorisation_trigger_request.py index 9bd7d7c8..4b35f75c 100644 --- a/backend/categorisation/categorisation_trigger_request.py +++ b/backend/categorisation/categorisation_trigger_request.py @@ -5,7 +5,7 @@ from pydantic import BaseModel class CategorisationTriggerRequest(BaseModel): portfolio_id: int - plans_to_consider: Optional[List[int]] = None + scenarios_to_consider: Optional[List[int]] = None plan_priority_order: Optional[List[int]] = None diff --git a/backend/categorisation/handler/handler.py b/backend/categorisation/handler/handler.py index ee0e7a7d..dc10fa4e 100644 --- a/backend/categorisation/handler/handler.py +++ b/backend/categorisation/handler/handler.py @@ -27,7 +27,7 @@ def handler(event: Mapping[str, Any], context: Any) -> None: process_portfolio( payload.portfolio_id, - payload.plans_to_consider, + payload.scenarios_to_consider, payload.plan_priority_order, ) diff --git a/backend/categorisation/local_handler/invoke_local_lambda.py b/backend/categorisation/local_handler/invoke_local_lambda.py index 23e5fda2..ce599ca9 100644 --- a/backend/categorisation/local_handler/invoke_local_lambda.py +++ b/backend/categorisation/local_handler/invoke_local_lambda.py @@ -10,7 +10,7 @@ payload = { "body": json.dumps( { "portfolio_id": 556, - "plans_to_consider": [1589319, 1589320], + "scenarios_to_consider": [1040, 1041], "plan_priority_order": [], } ) diff --git a/backend/categorisation/processor.py b/backend/categorisation/processor.py index b07f1c3b..e017c069 100644 --- a/backend/categorisation/processor.py +++ b/backend/categorisation/processor.py @@ -6,6 +6,7 @@ from backend.app.db.functions.recommendations_functions import ( get_default_plan_ids_for_property, get_plans_by_ids, get_plans_by_portfolio_id, + get_plans_by_scenario_ids, get_scenarios_by_portfolio_id, set_plan_and_scenario_default, ) @@ -19,12 +20,12 @@ logger = setup_logger() def process_portfolio( portfolio_id: int, - plans_to_consider: Optional[List[int]] = None, + scenarios_to_consider: Optional[List[int]] = None, plan_priority_order: Optional[List[int]] = None, ) -> None: logger.info(f"Processing portfolio {portfolio_id}") - plans: List[Plan] = _load_plans_for_portfolio(portfolio_id, plans_to_consider) + plans: List[Plan] = _load_plans_for_portfolio(portfolio_id, scenarios_to_consider) plans_by_property: Dict[int, List[Plan]] = _group_plans_by_property(plans) @@ -88,15 +89,17 @@ def choose_cheapest_relevant_plan( def _load_plans_for_portfolio( - portfolio_id: int, plans_to_consider: Optional[List[int]] = None + portfolio_id: int, scenarios_to_consider: Optional[List[int]] = None ) -> List[Plan]: - if plans_to_consider: - if len(plans_to_consider) < 2: - raise ValueError("Cannot run auto categorisation for fewer than 2 plans") + if scenarios_to_consider: + if len(scenarios_to_consider) < 2: + raise ValueError( + "Cannot run auto categorisation for fewer than 2 scenarios" + ) - logger.info(f"Getting {len(plans_to_consider)} Plans") - plan_models: List[PlanModel] = get_plans_by_ids(plans_to_consider) + logger.info(f"Getting {len(scenarios_to_consider)} plans") + plan_models: List[PlanModel] = get_plans_by_scenario_ids(scenarios_to_consider) else: logger.info( From ce8c1d23e6c8081b85f5dc2d59f71ca62df6a14f Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 18 Feb 2026 17:24:17 +0000 Subject: [PATCH 15/46] =?UTF-8?q?priority=20list=20is=20scenarios=20not=20?= =?UTF-8?q?plans=20=F0=9F=9F=A5?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../categorisation/categorisation_trigger_request.py | 2 +- backend/categorisation/handler/handler.py | 2 +- backend/categorisation/processor.py | 11 +++++------ .../tests/test_prioritised_plan_selected.py | 12 ++++++------ 4 files changed, 13 insertions(+), 14 deletions(-) diff --git a/backend/categorisation/categorisation_trigger_request.py b/backend/categorisation/categorisation_trigger_request.py index 4b35f75c..fbc2328b 100644 --- a/backend/categorisation/categorisation_trigger_request.py +++ b/backend/categorisation/categorisation_trigger_request.py @@ -6,7 +6,7 @@ class CategorisationTriggerRequest(BaseModel): portfolio_id: int scenarios_to_consider: Optional[List[int]] = None - plan_priority_order: Optional[List[int]] = None + scenario_priority_order: Optional[List[int]] = None # {"portfolio_id": 556, "plans_to_consider": [1589319,1589320], "plan_priority_order": [1589319,1589320]} diff --git a/backend/categorisation/handler/handler.py b/backend/categorisation/handler/handler.py index dc10fa4e..9fb235d5 100644 --- a/backend/categorisation/handler/handler.py +++ b/backend/categorisation/handler/handler.py @@ -28,7 +28,7 @@ def handler(event: Mapping[str, Any], context: Any) -> None: process_portfolio( payload.portfolio_id, payload.scenarios_to_consider, - payload.plan_priority_order, + payload.scenario_priority_order, ) except Exception as e: diff --git a/backend/categorisation/processor.py b/backend/categorisation/processor.py index e017c069..fd0d9c89 100644 --- a/backend/categorisation/processor.py +++ b/backend/categorisation/processor.py @@ -4,7 +4,6 @@ from typing import Dict, List, Optional from backend.app.db.functions.recommendations_functions import ( bulk_update_plans, get_default_plan_ids_for_property, - get_plans_by_ids, get_plans_by_portfolio_id, get_plans_by_scenario_ids, get_scenarios_by_portfolio_id, @@ -21,7 +20,7 @@ logger = setup_logger() def process_portfolio( portfolio_id: int, scenarios_to_consider: Optional[List[int]] = None, - plan_priority_order: Optional[List[int]] = None, + scenario_priority_order: Optional[List[int]] = None, ) -> None: logger.info(f"Processing portfolio {portfolio_id}") @@ -35,7 +34,7 @@ def process_portfolio( raise ValueError(f"No plans for property {property_id}") cheapest_plan = choose_cheapest_relevant_plan( - property_plans, plan_priority_order + property_plans, scenario_priority_order ) # Unset existing default(s) in case they are outside the plans to consider @@ -49,9 +48,9 @@ def process_portfolio( def choose_cheapest_relevant_plan( - plans: List[Plan], plan_priority_order: Optional[List[int]] = None + plans: List[Plan], scenario_priority_order: Optional[List[int]] = None ) -> Plan: - plan_priority_order = plan_priority_order or [] + scenario_priority_order = scenario_priority_order or [] eligible_plans: List[Plan] = [plan for plan in plans if plan.is_compliant] or plans if not eligible_plans: @@ -80,7 +79,7 @@ def choose_cheapest_relevant_plan( if (plan.record.cost_of_works or float("inf")) == min_cost ] - for priority_plan_id in plan_priority_order: + for priority_plan_id in scenario_priority_order: for plan in cheapest_plans: if plan.id == priority_plan_id: return plan diff --git a/backend/categorisation/tests/test_prioritised_plan_selected.py b/backend/categorisation/tests/test_prioritised_plan_selected.py index 5424dd5e..74eb8c69 100644 --- a/backend/categorisation/tests/test_prioritised_plan_selected.py +++ b/backend/categorisation/tests/test_prioritised_plan_selected.py @@ -39,7 +39,7 @@ def make_scenario(name: str, created_at: datetime, is_default: bool) -> Scenario multi_plan=False, is_default=is_default, ) - return Scenario(record=record, id=1 if is_default else 2) + return Scenario(record=record, id=3 if is_default else 4) def make_plan( @@ -54,17 +54,17 @@ def make_plan( ) -def test_prioritised_plan_selected(created_at_datetime: datetime) -> None: +def test_prioritised_scenario_selected(created_at_datetime: datetime) -> None: # arrange epc_c_plan = make_plan(created_at_datetime, True, name="EPC C") minor_works_plan = make_plan(created_at_datetime, False, name="EPC C - Minor Works") - plan_priority_order: List[int] = [2, 1] + scenario_priority_order: List[int] = [4, 3] expected_default_plan_id = 2 # act actual_default_plan = choose_cheapest_relevant_plan( plans=[epc_c_plan, minor_works_plan], - plan_priority_order=plan_priority_order, + scenario_priority_order=scenario_priority_order, ) # assert @@ -81,13 +81,13 @@ def test_cheapest_plan_returned_if_not_in_priority_list( minor_works_plan = make_plan( created_at_datetime, False, cost_of_works=100.0, name="EPC C - Minor Works" ) - plan_priority_order: List[int] = [1, 3] + scenario_priority_order: List[int] = [3, 5] expected_default_plan_id = 2 # act actual_default_plan = choose_cheapest_relevant_plan( plans=[epc_c_plan, minor_works_plan], - plan_priority_order=plan_priority_order, + scenario_priority_order=scenario_priority_order, ) # assert From c1aa5716beed1fd9de3f4266918cf977c712e957 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 18 Feb 2026 17:26:22 +0000 Subject: [PATCH 16/46] =?UTF-8?q?priority=20list=20is=20scenarios=20not=20?= =?UTF-8?q?plans=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- backend/categorisation/processor.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/categorisation/processor.py b/backend/categorisation/processor.py index fd0d9c89..ca58fb9d 100644 --- a/backend/categorisation/processor.py +++ b/backend/categorisation/processor.py @@ -79,9 +79,9 @@ def choose_cheapest_relevant_plan( if (plan.record.cost_of_works or float("inf")) == min_cost ] - for priority_plan_id in scenario_priority_order: + for priority_scenario_id in scenario_priority_order: for plan in cheapest_plans: - if plan.id == priority_plan_id: + if plan.scenario.id == priority_scenario_id: return plan return cheapest_plans[0] From 5ffa7290782ee5c2fb904b1c81453de6fc6074a8 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Wed, 18 Feb 2026 17:35:31 +0000 Subject: [PATCH 17/46] rename variable in invoke local lambda --- backend/categorisation/local_handler/invoke_local_lambda.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/categorisation/local_handler/invoke_local_lambda.py b/backend/categorisation/local_handler/invoke_local_lambda.py index ce599ca9..a53e0d8e 100644 --- a/backend/categorisation/local_handler/invoke_local_lambda.py +++ b/backend/categorisation/local_handler/invoke_local_lambda.py @@ -11,7 +11,7 @@ payload = { { "portfolio_id": 556, "scenarios_to_consider": [1040, 1041], - "plan_priority_order": [], + "scenarios_priority_order": [], } ) } From b715a60d4a8f6e21aef7043385c44d805c810f2e Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Thu, 19 Feb 2026 09:25:55 +0000 Subject: [PATCH 18/46] typo in log --- backend/categorisation/processor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/categorisation/processor.py b/backend/categorisation/processor.py index ca58fb9d..e1c0c6ff 100644 --- a/backend/categorisation/processor.py +++ b/backend/categorisation/processor.py @@ -165,4 +165,4 @@ def _update_default_flags(plans: List[Plan], cheapest_plan: Plan) -> None: logger.info("Successfully updated Plan default values") else: - logger.info("All plan default values already correct. Not udpating") + logger.info("All plan default values already correct. Not updating") From dae74d2f8b9ec93b7859a2bff4207055d374d295 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Thu, 19 Feb 2026 12:18:22 +0000 Subject: [PATCH 19/46] Fix bug where default plans were not being unset if they weren't included in scenarios to be considered --- .../db/functions/recommendations_functions.py | 21 ++++++++- .../local_handler/invoke_local_lambda.py | 2 +- backend/categorisation/processor.py | 46 ++++++++++++++----- 3 files changed, 56 insertions(+), 13 deletions(-) diff --git a/backend/app/db/functions/recommendations_functions.py b/backend/app/db/functions/recommendations_functions.py index 6f7dd41f..ff9b9dc8 100644 --- a/backend/app/db/functions/recommendations_functions.py +++ b/backend/app/db/functions/recommendations_functions.py @@ -639,6 +639,13 @@ def get_plans_by_scenario_ids(ids: List[int]) -> List[PlanModel]: return session_any.exec(stmt).scalars().all() +def get_plan_ids_by_scenario_ids(scenario_ids: List[int]) -> List[int]: + stmt = select(PlanModel.id).where(PlanModel.scenario_id.in_(scenario_ids)) + with db_read_session() as session: + session_any: Any = session # Typehint as Any to satisfy Pylance... + return session_any.exec(stmt).scalars().all() + + def get_scenarios_by_portfolio_id(portfolio_id: int) -> List[ScenarioModel]: stmt = select(ScenarioModel).where(ScenarioModel.portfolio_id == portfolio_id) with db_read_session() as session: @@ -657,7 +664,19 @@ def get_default_plan_ids_for_property(property_id: int) -> List[int]: # This should in reality always return exactly 1 ID, but there's currently # no database constraint to enforce that, so account for 0 or >1 stmt = select(PlanModel.id).where( - PlanModel.property_id == property_id and PlanModel.is_default + (PlanModel.property_id == property_id) & (PlanModel.is_default == True) + ) + with db_read_session() as session: + session_any: Any = session # Typehint as Any to satisfy Pylance... + return session_any.exec(stmt).scalars().all() + + +def get_default_scenario_ids_for_portfolio(portfolio_id: int) -> List[int]: + # This should in reality always return exactly 1 ID, but there's currently + # no database constraint to enforce that, so account for 0 or >1 + stmt = select(ScenarioModel.id).where( + (ScenarioModel.portfolio_id == portfolio_id) + & (ScenarioModel.is_default == True) ) with db_read_session() as session: session_any: Any = session # Typehint as Any to satisfy Pylance... diff --git a/backend/categorisation/local_handler/invoke_local_lambda.py b/backend/categorisation/local_handler/invoke_local_lambda.py index a53e0d8e..7d092d67 100644 --- a/backend/categorisation/local_handler/invoke_local_lambda.py +++ b/backend/categorisation/local_handler/invoke_local_lambda.py @@ -10,7 +10,7 @@ payload = { "body": json.dumps( { "portfolio_id": 556, - "scenarios_to_consider": [1040, 1041], + "scenarios_to_consider": [], "scenarios_priority_order": [], } ) diff --git a/backend/categorisation/processor.py b/backend/categorisation/processor.py index e1c0c6ff..f619d5fd 100644 --- a/backend/categorisation/processor.py +++ b/backend/categorisation/processor.py @@ -3,7 +3,8 @@ from typing import Dict, List, Optional from backend.app.db.functions.recommendations_functions import ( bulk_update_plans, - get_default_plan_ids_for_property, + get_default_scenario_ids_for_portfolio, + get_plan_ids_by_scenario_ids, get_plans_by_portfolio_id, get_plans_by_scenario_ids, get_scenarios_by_portfolio_id, @@ -24,6 +25,17 @@ def process_portfolio( ) -> None: logger.info(f"Processing portfolio {portfolio_id}") + if scenarios_to_consider: + if len(scenarios_to_consider) < 2: + raise ValueError( + "Cannot run auto categorisation for fewer than 2 scenarios" + ) + + if scenarios_to_consider is not None: + _unset_defaults_for_scenarios_not_being_considered( + portfolio_id, scenarios_to_consider + ) + plans: List[Plan] = _load_plans_for_portfolio(portfolio_id, scenarios_to_consider) plans_by_property: Dict[int, List[Plan]] = _group_plans_by_property(plans) @@ -37,11 +49,6 @@ def process_portfolio( property_plans, scenario_priority_order ) - # Unset existing default(s) in case they are outside the plans to consider - default_plan_ids: List[int] = get_default_plan_ids_for_property(property_id) - for id in default_plan_ids: - set_plan_and_scenario_default(id, False) - _update_default_flags( property_plans, cheapest_plan ) # TODO: we have already unset existing default(s), so this method can probably be a bit simpler now @@ -87,16 +94,33 @@ def choose_cheapest_relevant_plan( return cheapest_plans[0] +def _unset_defaults_for_scenarios_not_being_considered( + portfolio_id: int, scenarios_to_consider: List[int] +) -> None: + default_scenario_ids: List[int] = get_default_scenario_ids_for_portfolio( + portfolio_id + ) + scenarios_to_unset_default: List[int] = [] + + for id in default_scenario_ids: + if id not in scenarios_to_consider: + scenarios_to_unset_default.append(id) + + logger.info(f"Scenarios to unset defaults: {scenarios_to_unset_default}") + + if len(scenarios_to_unset_default) > 0: + plans_to_unset_default: List[int] = get_plan_ids_by_scenario_ids( + scenarios_to_unset_default + ) + for plan_id in plans_to_unset_default: + set_plan_and_scenario_default(plan_id, False) # TODO: do this in batch + + def _load_plans_for_portfolio( portfolio_id: int, scenarios_to_consider: Optional[List[int]] = None ) -> List[Plan]: if scenarios_to_consider: - if len(scenarios_to_consider) < 2: - raise ValueError( - "Cannot run auto categorisation for fewer than 2 scenarios" - ) - logger.info(f"Getting {len(scenarios_to_consider)} plans") plan_models: List[PlanModel] = get_plans_by_scenario_ids(scenarios_to_consider) From 9d12eef0e587537932a55fdb7726300380b4c60a Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Thu, 19 Feb 2026 12:22:19 +0000 Subject: [PATCH 20/46] Remove unused db functions --- .../db/functions/recommendations_functions.py | 25 ------------------- 1 file changed, 25 deletions(-) diff --git a/backend/app/db/functions/recommendations_functions.py b/backend/app/db/functions/recommendations_functions.py index ff9b9dc8..7ffcf603 100644 --- a/backend/app/db/functions/recommendations_functions.py +++ b/backend/app/db/functions/recommendations_functions.py @@ -625,13 +625,6 @@ def get_plans_by_portfolio_id(portfolio_id: int) -> List[PlanModel]: return session_any.exec(stmt).scalars().all() -def get_plans_by_ids(ids: List[int]) -> List[PlanModel]: - stmt = select(PlanModel).where(PlanModel.id.in_(ids)) - with db_read_session() as session: - session_any: Any = session # Typehint as Any to satisfy Pylance... - return session_any.exec(stmt).scalars().all() - - def get_plans_by_scenario_ids(ids: List[int]) -> List[PlanModel]: stmt = select(PlanModel).where(PlanModel.scenario_id.in_(ids)) with db_read_session() as session: @@ -653,24 +646,6 @@ def get_scenarios_by_portfolio_id(portfolio_id: int) -> List[ScenarioModel]: return session_any.exec(stmt).scalars().all() -def get_scenario(scenario_id: int) -> Optional[ScenarioModel]: - stmt = select(ScenarioModel).where(ScenarioModel.id == scenario_id) - with db_read_session() as session: - session_any: Any = session # Typehint as Any to satisfy Pylance... - return session_any.exec(stmt).scalar_one_or_none() - - -def get_default_plan_ids_for_property(property_id: int) -> List[int]: - # This should in reality always return exactly 1 ID, but there's currently - # no database constraint to enforce that, so account for 0 or >1 - stmt = select(PlanModel.id).where( - (PlanModel.property_id == property_id) & (PlanModel.is_default == True) - ) - with db_read_session() as session: - session_any: Any = session # Typehint as Any to satisfy Pylance... - return session_any.exec(stmt).scalars().all() - - def get_default_scenario_ids_for_portfolio(portfolio_id: int) -> List[int]: # This should in reality always return exactly 1 ID, but there's currently # no database constraint to enforce that, so account for 0 or >1 From 63bef436d086f791c10acbdb79371d585d5016ee Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Thu, 19 Feb 2026 12:44:41 +0000 Subject: [PATCH 21/46] do batch update per portfolio not per property --- .../local_handler/invoke_local_lambda.py | 2 +- backend/categorisation/processor.py | 45 +++++++++++-------- 2 files changed, 28 insertions(+), 19 deletions(-) diff --git a/backend/categorisation/local_handler/invoke_local_lambda.py b/backend/categorisation/local_handler/invoke_local_lambda.py index 7d092d67..127d2575 100644 --- a/backend/categorisation/local_handler/invoke_local_lambda.py +++ b/backend/categorisation/local_handler/invoke_local_lambda.py @@ -10,7 +10,7 @@ payload = { "body": json.dumps( { "portfolio_id": 556, - "scenarios_to_consider": [], + "scenarios_to_consider": [1039, 1041], "scenarios_priority_order": [], } ) diff --git a/backend/categorisation/processor.py b/backend/categorisation/processor.py index f619d5fd..966ecbf5 100644 --- a/backend/categorisation/processor.py +++ b/backend/categorisation/processor.py @@ -1,5 +1,5 @@ from collections import defaultdict -from typing import Dict, List, Optional +from typing import Dict, List, Optional, Tuple from backend.app.db.functions.recommendations_functions import ( bulk_update_plans, @@ -40,6 +40,9 @@ def process_portfolio( plans_by_property: Dict[int, List[Plan]] = _group_plans_by_property(plans) + updated_plan_models: List[PlanModel] = [] + updated_scenario_models: List[ScenarioModel] = [] + for property_id, property_plans in plans_by_property.items(): if not property_plans: @@ -49,9 +52,16 @@ def process_portfolio( property_plans, scenario_priority_order ) - _update_default_flags( - property_plans, cheapest_plan - ) # TODO: we have already unset existing default(s), so this method can probably be a bit simpler now + updated_property_plan_models, updated_property_scenario_models = ( + _update_plan_and_scenario_objects(property_plans, cheapest_plan) + ) + + updated_plan_models.extend(updated_property_plan_models) + updated_scenario_models.extend(updated_property_scenario_models) + + if len(updated_plan_models) > 0: + bulk_update_plans(updated_plan_models, updated_scenario_models) + logger.info("Successfully updated Plan default values in database") def choose_cheapest_relevant_plan( @@ -106,7 +116,9 @@ def _unset_defaults_for_scenarios_not_being_considered( if id not in scenarios_to_consider: scenarios_to_unset_default.append(id) - logger.info(f"Scenarios to unset defaults: {scenarios_to_unset_default}") + logger.info( + f"Unsetting {scenarios_to_unset_default} as default scenario(s) as not included in provided list of scenarios to consider" + ) if len(scenarios_to_unset_default) > 0: plans_to_unset_default: List[int] = get_plan_ids_by_scenario_ids( @@ -164,7 +176,9 @@ def _group_plans_by_property(plans: List[Plan]) -> Dict[int, List[Plan]]: return grouped -def _update_default_flags(plans: List[Plan], cheapest_plan: Plan) -> None: +def _update_plan_and_scenario_objects( + plans: List[Plan], cheapest_plan: Plan +) -> Tuple[List[PlanModel], List[ScenarioModel]]: plans_to_update: List[Plan] = [] for plan in plans: @@ -176,17 +190,12 @@ def _update_default_flags(plans: List[Plan], cheapest_plan: Plan) -> None: plan.set_default(should_be_default) plans_to_update.append(plan) - if plans_to_update: - plan_models: List[PlanModel] = [] - scenario_models: List[ScenarioModel] = [] + plan_models: List[PlanModel] = [] + scenario_models: List[ScenarioModel] = [] - for plan in plans_to_update: - plan_model, scenario_model = plan.to_sqlalchemy() - plan_models.append(plan_model) - scenario_models.append(scenario_model) + for plan in plans_to_update: + plan_model, scenario_model = plan.to_sqlalchemy() + plan_models.append(plan_model) + scenario_models.append(scenario_model) - bulk_update_plans(plan_models, scenario_models) - logger.info("Successfully updated Plan default values") - - else: - logger.info("All plan default values already correct. Not updating") + return (plan_models, scenario_models) From 6dcb4d1d92ce12b9e157e8ad3ea494058391d5bf Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Thu, 19 Feb 2026 17:03:40 +0000 Subject: [PATCH 22/46] additional logging --- .../categorisation_trigger_request.py | 2 +- .../local_handler/invoke_local_lambda.py | 6 +++--- backend/categorisation/local_runner.py | 6 ++++-- backend/categorisation/processor.py | 20 ++++++++++--------- 4 files changed, 19 insertions(+), 15 deletions(-) diff --git a/backend/categorisation/categorisation_trigger_request.py b/backend/categorisation/categorisation_trigger_request.py index fbc2328b..44ac0ff1 100644 --- a/backend/categorisation/categorisation_trigger_request.py +++ b/backend/categorisation/categorisation_trigger_request.py @@ -9,4 +9,4 @@ class CategorisationTriggerRequest(BaseModel): scenario_priority_order: Optional[List[int]] = None -# {"portfolio_id": 556, "plans_to_consider": [1589319,1589320], "plan_priority_order": [1589319,1589320]} +# {"portfolio_id": 556, "scenarios_to_consider": [1039,1041], "scenario_priority_order": [1041,1039]} diff --git a/backend/categorisation/local_handler/invoke_local_lambda.py b/backend/categorisation/local_handler/invoke_local_lambda.py index 127d2575..1446a1e3 100644 --- a/backend/categorisation/local_handler/invoke_local_lambda.py +++ b/backend/categorisation/local_handler/invoke_local_lambda.py @@ -9,9 +9,9 @@ payload = { { "body": json.dumps( { - "portfolio_id": 556, - "scenarios_to_consider": [1039, 1041], - "scenarios_priority_order": [], + "portfolio_id": 569, + "scenarios_to_consider": [1069, 1060], + "scenario_priority_order": [1069, 1060], } ) } diff --git a/backend/categorisation/local_runner.py b/backend/categorisation/local_runner.py index 599cbbbb..f4718ffc 100644 --- a/backend/categorisation/local_runner.py +++ b/backend/categorisation/local_runner.py @@ -2,9 +2,11 @@ from backend.categorisation.processor import process_portfolio def main() -> None: - portfolio_id = 556 + portfolio_id = 569 + scenarios_to_consider = [1069, 1060] + scenario_priority_order = [1069, 1060] - process_portfolio(portfolio_id) + process_portfolio(portfolio_id, scenarios_to_consider, scenario_priority_order) if __name__ == "__main__": diff --git a/backend/categorisation/processor.py b/backend/categorisation/processor.py index 966ecbf5..5ed75d8f 100644 --- a/backend/categorisation/processor.py +++ b/backend/categorisation/processor.py @@ -37,8 +37,10 @@ def process_portfolio( ) plans: List[Plan] = _load_plans_for_portfolio(portfolio_id, scenarios_to_consider) + logger.info(f"Successfully loaded {len(plans)}") plans_by_property: Dict[int, List[Plan]] = _group_plans_by_property(plans) + logger.info("Successfully grouped plans by property") updated_plan_models: List[PlanModel] = [] updated_scenario_models: List[ScenarioModel] = [] @@ -51,6 +53,7 @@ def process_portfolio( cheapest_plan = choose_cheapest_relevant_plan( property_plans, scenario_priority_order ) + logger.info(f"Successfully found cheapest plan for Property {property_id}") updated_property_plan_models, updated_property_scenario_models = ( _update_plan_and_scenario_objects(property_plans, cheapest_plan) @@ -60,6 +63,7 @@ def process_portfolio( updated_scenario_models.extend(updated_property_scenario_models) if len(updated_plan_models) > 0: + logger.info(f"Updating {len(updated_plan_models)} Plans in database") bulk_update_plans(updated_plan_models, updated_scenario_models) logger.info("Successfully updated Plan default values in database") @@ -116,9 +120,10 @@ def _unset_defaults_for_scenarios_not_being_considered( if id not in scenarios_to_consider: scenarios_to_unset_default.append(id) - logger.info( - f"Unsetting {scenarios_to_unset_default} as default scenario(s) as not included in provided list of scenarios to consider" - ) + if len(scenarios_to_unset_default) > 0: + logger.info( + f"Unsetting {scenarios_to_unset_default} as default scenario(s) as not included in provided list of scenarios to consider" + ) if len(scenarios_to_unset_default) > 0: plans_to_unset_default: List[int] = get_plan_ids_by_scenario_ids( @@ -133,9 +138,9 @@ def _load_plans_for_portfolio( ) -> List[Plan]: if scenarios_to_consider: - logger.info(f"Getting {len(scenarios_to_consider)} plans") + logger.info(f"Getting plans for {len(scenarios_to_consider)} scenarios") plan_models: List[PlanModel] = get_plans_by_scenario_ids(scenarios_to_consider) - + logger.info(f"Got {len(plan_models)} plan models from database") else: logger.info( f"No list of Plans to consider provided. Getting all Plans for portfolio {portfolio_id}" @@ -159,11 +164,8 @@ def _load_plans_for_portfolio( plans.append( Plan.from_sqlalchemy(model, Scenario.from_sqlalchemy(scenario_model)) ) - logger.debug( - f"Successfully mapped plan {model.id} and scenario {scenario_model.id} to domain object" - ) - logger.debug(f"Got {len(plans)} plans from database") + logger.info(f"Got {len(plans)} Plans") return plans From a8d3ce599d3b5dd6ea4e092375506aa4355c7dc6 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Fri, 20 Feb 2026 10:11:31 +0000 Subject: [PATCH 23/46] =?UTF-8?q?handle=20all=20plans=20having=20zero=20co?= =?UTF-8?q?st=20=F0=9F=9F=A5?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../tests/test_prioritised_plan_selected.py | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/backend/categorisation/tests/test_prioritised_plan_selected.py b/backend/categorisation/tests/test_prioritised_plan_selected.py index 74eb8c69..7544eb9c 100644 --- a/backend/categorisation/tests/test_prioritised_plan_selected.py +++ b/backend/categorisation/tests/test_prioritised_plan_selected.py @@ -92,3 +92,24 @@ def test_cheapest_plan_returned_if_not_in_priority_list( # assert assert actual_default_plan.id == expected_default_plan_id + + +def test_all_plans_zero_cost__highest_priority_returned( + created_at_datetime: datetime, +) -> None: + # arrange + epc_c_plan = make_plan(created_at_datetime, True, cost_of_works=0.0, name="EPC C") + minor_works_plan = make_plan( + created_at_datetime, False, cost_of_works=0.0, name="EPC C - Minor Works" + ) + scenario_priority_order: List[int] = [4, 3] + expected_default_plan_id = 2 + + # act + actual_default_plan = choose_cheapest_relevant_plan( + plans=[epc_c_plan, minor_works_plan], + scenario_priority_order=scenario_priority_order, + ) + + # assert + assert actual_default_plan.id == expected_default_plan_id From 0e279b15cec06d80bee07e17af73cfc6151b444b Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Fri, 20 Feb 2026 10:36:22 +0000 Subject: [PATCH 24/46] =?UTF-8?q?handle=20all=20plans=20having=20zero=20co?= =?UTF-8?q?st=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- backend/categorisation/processor.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/backend/categorisation/processor.py b/backend/categorisation/processor.py index 5ed75d8f..590d064f 100644 --- a/backend/categorisation/processor.py +++ b/backend/categorisation/processor.py @@ -94,11 +94,15 @@ def choose_cheapest_relevant_plan( for plan in eligible_plans ) - cheapest_plans: List[Plan] = [ - plan - for plan in eligible_plans - if (plan.record.cost_of_works or float("inf")) == min_cost - ] + if all(p.record.cost_of_works == 0 for p in eligible_plans): + cheapest_plans = eligible_plans + + else: + cheapest_plans: List[Plan] = [ + plan + for plan in eligible_plans + if (plan.record.cost_of_works or float("inf")) == min_cost + ] for priority_scenario_id in scenario_priority_order: for plan in cheapest_plans: From cb55338f39eafdbfb03009bbfffc094c883cb5ad Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Fri, 20 Feb 2026 10:38:29 +0000 Subject: [PATCH 25/46] =?UTF-8?q?handle=20all=20plans=20having=20null=20co?= =?UTF-8?q?st=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../tests/test_prioritised_plan_selected.py | 30 +++++++++++++++++-- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/backend/categorisation/tests/test_prioritised_plan_selected.py b/backend/categorisation/tests/test_prioritised_plan_selected.py index 7544eb9c..e2af6a63 100644 --- a/backend/categorisation/tests/test_prioritised_plan_selected.py +++ b/backend/categorisation/tests/test_prioritised_plan_selected.py @@ -1,5 +1,5 @@ from datetime import datetime -from typing import List +from typing import List, Optional import pytest from backend.app.domain.classes.plan import Plan @@ -16,7 +16,7 @@ def created_at_datetime() -> datetime: def make_plan_record( - created_at: datetime, default: bool, cost_of_works: float = 500.0 + created_at: datetime, default: bool, cost_of_works: Optional[float] = 500.0 ) -> PlanRecord: return PlanRecord( property_id=1, @@ -43,7 +43,10 @@ def make_scenario(name: str, created_at: datetime, is_default: bool) -> Scenario def make_plan( - created_at: datetime, default: bool, cost_of_works: float = 500.0, name: str = "" + created_at: datetime, + default: bool, + cost_of_works: Optional[float] = 500.0, + name: str = "", ) -> Plan: scenario = make_scenario(name, created_at, default) plan_id = 1 if default else 2 @@ -113,3 +116,24 @@ def test_all_plans_zero_cost__highest_priority_returned( # assert assert actual_default_plan.id == expected_default_plan_id + + +def test_all_plans_null_cost__highest_priority_returned( + created_at_datetime: datetime, +) -> None: + # arrange + epc_c_plan = make_plan(created_at_datetime, True, cost_of_works=None, name="EPC C") + minor_works_plan = make_plan( + created_at_datetime, False, cost_of_works=None, name="EPC C - Minor Works" + ) + scenario_priority_order: List[int] = [4, 3] + expected_default_plan_id = 2 + + # act + actual_default_plan = choose_cheapest_relevant_plan( + plans=[epc_c_plan, minor_works_plan], + scenario_priority_order=scenario_priority_order, + ) + + # assert + assert actual_default_plan.id == expected_default_plan_id From 47de308bf353207990a31657deed2303b33541d8 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Fri, 20 Feb 2026 10:39:16 +0000 Subject: [PATCH 26/46] reformatting --- backend/categorisation/processor.py | 1 - 1 file changed, 1 deletion(-) diff --git a/backend/categorisation/processor.py b/backend/categorisation/processor.py index 590d064f..e5d69dcf 100644 --- a/backend/categorisation/processor.py +++ b/backend/categorisation/processor.py @@ -96,7 +96,6 @@ def choose_cheapest_relevant_plan( if all(p.record.cost_of_works == 0 for p in eligible_plans): cheapest_plans = eligible_plans - else: cheapest_plans: List[Plan] = [ plan From 481bd1197afcd729a437fc0ec5795f0479885c21 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Fri, 20 Feb 2026 10:40:33 +0000 Subject: [PATCH 27/46] put test portfolio back into invoke lambda script --- backend/categorisation/local_handler/invoke_local_lambda.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/backend/categorisation/local_handler/invoke_local_lambda.py b/backend/categorisation/local_handler/invoke_local_lambda.py index 1446a1e3..5ed23c2d 100644 --- a/backend/categorisation/local_handler/invoke_local_lambda.py +++ b/backend/categorisation/local_handler/invoke_local_lambda.py @@ -9,9 +9,9 @@ payload = { { "body": json.dumps( { - "portfolio_id": 569, - "scenarios_to_consider": [1069, 1060], - "scenario_priority_order": [1069, 1060], + "portfolio_id": 556, + "scenarios_to_consider": [], + "scenario_priority_order": [], } ) } From b4583d3c8b8890879c1b68eb3b1c01c6717a2f30 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Fri, 20 Feb 2026 10:42:03 +0000 Subject: [PATCH 28/46] put test portfolio back in local runner --- backend/categorisation/local_runner.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/backend/categorisation/local_runner.py b/backend/categorisation/local_runner.py index f4718ffc..7de55bc0 100644 --- a/backend/categorisation/local_runner.py +++ b/backend/categorisation/local_runner.py @@ -1,12 +1,18 @@ +from typing import List + from backend.categorisation.processor import process_portfolio def main() -> None: - portfolio_id = 569 - scenarios_to_consider = [1069, 1060] - scenario_priority_order = [1069, 1060] + portfolio_id = 556 + scenarios_to_consider: List[int] = [] + scenario_priority_order: List[int] = [] - process_portfolio(portfolio_id, scenarios_to_consider, scenario_priority_order) + process_portfolio( + portfolio_id=portfolio_id, + scenarios_to_consider=scenarios_to_consider, + scenario_priority_order=scenario_priority_order, + ) if __name__ == "__main__": From 678de56def8a32d9be9fd7a35188b438feb602ce Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Fri, 20 Feb 2026 11:24:17 +0000 Subject: [PATCH 29/46] =?UTF-8?q?handle=20some=20plans=20having=20zero=20c?= =?UTF-8?q?ost=20=F0=9F=9F=A5?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../tests/test_prioritised_plan_selected.py | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/backend/categorisation/tests/test_prioritised_plan_selected.py b/backend/categorisation/tests/test_prioritised_plan_selected.py index e2af6a63..5ddc7b8f 100644 --- a/backend/categorisation/tests/test_prioritised_plan_selected.py +++ b/backend/categorisation/tests/test_prioritised_plan_selected.py @@ -118,6 +118,27 @@ def test_all_plans_zero_cost__highest_priority_returned( assert actual_default_plan.id == expected_default_plan_id +def test_some_plans_zero_cost__cheapest_returned( + created_at_datetime: datetime, +) -> None: + # arrange + epc_c_plan = make_plan(created_at_datetime, True, cost_of_works=0.0, name="EPC C") + minor_works_plan = make_plan( + created_at_datetime, False, cost_of_works=50.0, name="EPC C - Minor Works" + ) + scenario_priority_order: List[int] = [4, 3] + expected_default_plan_id = 2 + + # act + actual_default_plan = choose_cheapest_relevant_plan( + plans=[epc_c_plan, minor_works_plan], + scenario_priority_order=scenario_priority_order, + ) + + # assert + assert actual_default_plan.id == expected_default_plan_id + + def test_all_plans_null_cost__highest_priority_returned( created_at_datetime: datetime, ) -> None: From ce94fb0573f46a0e91df5b390ac1f373b51ab2a2 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Fri, 20 Feb 2026 11:35:12 +0000 Subject: [PATCH 30/46] =?UTF-8?q?handle=20some=20plans=20having=20zero=20c?= =?UTF-8?q?ost=20=F0=9F=9F=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- backend/app/domain/classes/plan.py | 8 +++++ backend/categorisation/processor.py | 31 +++++++------------ .../tests/test_prioritised_plan_selected.py | 2 +- 3 files changed, 20 insertions(+), 21 deletions(-) diff --git a/backend/app/domain/classes/plan.py b/backend/app/domain/classes/plan.py index 7970abcd..351ea512 100644 --- a/backend/app/domain/classes/plan.py +++ b/backend/app/domain/classes/plan.py @@ -60,6 +60,14 @@ class Plan: case _: raise NotImplementedError + @property + def cost(self) -> float: + return ( + self.record.cost_of_works + if self.record.cost_of_works is not None + else float("inf") + ) + def to_sqlalchemy(self) -> PlanPersistence: scenario_record = self.scenario.record diff --git a/backend/categorisation/processor.py b/backend/categorisation/processor.py index e5d69dcf..e90c3b08 100644 --- a/backend/categorisation/processor.py +++ b/backend/categorisation/processor.py @@ -50,10 +50,13 @@ def process_portfolio( if not property_plans: raise ValueError(f"No plans for property {property_id}") - cheapest_plan = choose_cheapest_relevant_plan( - property_plans, scenario_priority_order - ) - logger.info(f"Successfully found cheapest plan for Property {property_id}") + try: + cheapest_plan = choose_cheapest_relevant_plan( + property_plans, scenario_priority_order + ) + except Exception: + logger.error(f"Failed to find cheapest plan for property {property_id}") + raise updated_property_plan_models, updated_property_scenario_models = ( _update_plan_and_scenario_objects(property_plans, cheapest_plan) @@ -85,23 +88,11 @@ def choose_cheapest_relevant_plan( f"All plans must have an ID, but found a plan with no ID: {plan}" ) - min_cost: float = min( - ( - plan.record.cost_of_works - if plan.record.cost_of_works is not None - else float("inf") - ) - for plan in eligible_plans - ) + min_cost: float = min(plan.cost for plan in eligible_plans) - if all(p.record.cost_of_works == 0 for p in eligible_plans): - cheapest_plans = eligible_plans - else: - cheapest_plans: List[Plan] = [ - plan - for plan in eligible_plans - if (plan.record.cost_of_works or float("inf")) == min_cost - ] + cheapest_plans: List[Plan] = [ + plan for plan in eligible_plans if plan.cost == min_cost + ] for priority_scenario_id in scenario_priority_order: for plan in cheapest_plans: diff --git a/backend/categorisation/tests/test_prioritised_plan_selected.py b/backend/categorisation/tests/test_prioritised_plan_selected.py index 5ddc7b8f..a9529a53 100644 --- a/backend/categorisation/tests/test_prioritised_plan_selected.py +++ b/backend/categorisation/tests/test_prioritised_plan_selected.py @@ -127,7 +127,7 @@ def test_some_plans_zero_cost__cheapest_returned( created_at_datetime, False, cost_of_works=50.0, name="EPC C - Minor Works" ) scenario_priority_order: List[int] = [4, 3] - expected_default_plan_id = 2 + expected_default_plan_id = 1 # act actual_default_plan = choose_cheapest_relevant_plan( From ec01e1d190b9eb645b9aa00dca34594b935ddf90 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Fri, 20 Feb 2026 13:39:07 +0000 Subject: [PATCH 31/46] only get most recently added plans for scenario --- .../app/db/functions/recommendations_functions.py | 15 +++++++++++++-- backend/categorisation/processor.py | 6 ++---- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/backend/app/db/functions/recommendations_functions.py b/backend/app/db/functions/recommendations_functions.py index 7ffcf603..900b5b9f 100644 --- a/backend/app/db/functions/recommendations_functions.py +++ b/backend/app/db/functions/recommendations_functions.py @@ -632,8 +632,19 @@ def get_plans_by_scenario_ids(ids: List[int]) -> List[PlanModel]: return session_any.exec(stmt).scalars().all() -def get_plan_ids_by_scenario_ids(scenario_ids: List[int]) -> List[int]: - stmt = select(PlanModel.id).where(PlanModel.scenario_id.in_(scenario_ids)) +def get_most_recent_plan_ids_by_scenario_ids(scenario_ids: List[int]) -> List[int]: + # NOTE: This statement works for Postgres only, because of the Distinct + stmt = ( + select(PlanModel.id) + .where(PlanModel.scenario_id.in_(scenario_ids)) + .distinct(PlanModel.scenario_id) + .order_by( + PlanModel.scenario_id, + PlanModel.created_at.desc(), + PlanModel.id.desc(), + ) + ) + with db_read_session() as session: session_any: Any = session # Typehint as Any to satisfy Pylance... return session_any.exec(stmt).scalars().all() diff --git a/backend/categorisation/processor.py b/backend/categorisation/processor.py index e90c3b08..ea12bc3b 100644 --- a/backend/categorisation/processor.py +++ b/backend/categorisation/processor.py @@ -4,7 +4,7 @@ from typing import Dict, List, Optional, Tuple from backend.app.db.functions.recommendations_functions import ( bulk_update_plans, get_default_scenario_ids_for_portfolio, - get_plan_ids_by_scenario_ids, + get_most_recent_plan_ids_by_scenario_ids, get_plans_by_portfolio_id, get_plans_by_scenario_ids, get_scenarios_by_portfolio_id, @@ -37,10 +37,8 @@ def process_portfolio( ) plans: List[Plan] = _load_plans_for_portfolio(portfolio_id, scenarios_to_consider) - logger.info(f"Successfully loaded {len(plans)}") plans_by_property: Dict[int, List[Plan]] = _group_plans_by_property(plans) - logger.info("Successfully grouped plans by property") updated_plan_models: List[PlanModel] = [] updated_scenario_models: List[ScenarioModel] = [] @@ -120,7 +118,7 @@ def _unset_defaults_for_scenarios_not_being_considered( ) if len(scenarios_to_unset_default) > 0: - plans_to_unset_default: List[int] = get_plan_ids_by_scenario_ids( + plans_to_unset_default: List[int] = get_most_recent_plan_ids_by_scenario_ids( scenarios_to_unset_default ) for plan_id in plans_to_unset_default: From 96fbd7f24c55eafb22d503fa6592d3c357237c99 Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Fri, 20 Feb 2026 15:26:40 +0000 Subject: [PATCH 32/46] ensure all defaults are unset before setting new ones, refactor of processor --- .../db/functions/recommendations_functions.py | 83 ++++++------ backend/categorisation/processor.py | 119 +++++++++--------- 2 files changed, 98 insertions(+), 104 deletions(-) diff --git a/backend/app/db/functions/recommendations_functions.py b/backend/app/db/functions/recommendations_functions.py index 900b5b9f..141ba2dd 100644 --- a/backend/app/db/functions/recommendations_functions.py +++ b/backend/app/db/functions/recommendations_functions.py @@ -1,5 +1,5 @@ -from typing import Any, Dict, List, Optional -from sqlalchemy import inspect, text, insert, delete, select, update +from typing import Any, Dict, List, Tuple +from sqlalchemy import inspect, text, insert, delete, select from sqlalchemy.orm import Session, Mapper from sqlalchemy.exc import SQLAlchemyError from sqlmodel import Session @@ -618,13 +618,6 @@ def clear_portfolio_in_batches( print("Portfolio cleared in batches.") -def get_plans_by_portfolio_id(portfolio_id: int) -> List[PlanModel]: - stmt = select(PlanModel).where(PlanModel.portfolio_id == portfolio_id) - with db_read_session() as session: - session_any: Any = session # Typehint as Any to satisfy Pylance... - return session_any.exec(stmt).scalars().all() - - def get_plans_by_scenario_ids(ids: List[int]) -> List[PlanModel]: stmt = select(PlanModel).where(PlanModel.scenario_id.in_(ids)) with db_read_session() as session: @@ -632,13 +625,36 @@ def get_plans_by_scenario_ids(ids: List[int]) -> List[PlanModel]: return session_any.exec(stmt).scalars().all() -def get_most_recent_plan_ids_by_scenario_ids(scenario_ids: List[int]) -> List[int]: +def get_most_recent_plans_by_portfolio_id(portfolio_id: int) -> List[PlanModel]: # NOTE: This statement works for Postgres only, because of the Distinct stmt = ( - select(PlanModel.id) - .where(PlanModel.scenario_id.in_(scenario_ids)) - .distinct(PlanModel.scenario_id) + select(PlanModel) + .where(PlanModel.portfolio_id == portfolio_id) + .distinct( + PlanModel.property_id, PlanModel.scenario_id + ) # one plan per property per scenario .order_by( + PlanModel.property_id, + PlanModel.scenario_id, + PlanModel.created_at.desc(), + PlanModel.id.desc(), + ) + ) + with db_read_session() as session: + session_any: Any = session # Typehint as Any to satisfy Pylance... + return session_any.exec(stmt).scalars().all() + + +def get_most_recent_plans_by_scenario_ids(scenario_ids: List[int]) -> List[PlanModel]: + # NOTE: This statement works for Postgres only, because of the Distinct + stmt = ( + select(PlanModel) + .where(PlanModel.scenario_id.in_(scenario_ids)) + .distinct( + PlanModel.property_id, PlanModel.scenario_id + ) # one plan per property per scenario + .order_by( + PlanModel.property_id, PlanModel.scenario_id, PlanModel.created_at.desc(), PlanModel.id.desc(), @@ -646,7 +662,7 @@ def get_most_recent_plan_ids_by_scenario_ids(scenario_ids: List[int]) -> List[in ) with db_read_session() as session: - session_any: Any = session # Typehint as Any to satisfy Pylance... + session_any: Any = session # Typehint as Any to satisfy Pylance return session_any.exec(stmt).scalars().all() @@ -657,39 +673,22 @@ def get_scenarios_by_portfolio_id(portfolio_id: int) -> List[ScenarioModel]: return session_any.exec(stmt).scalars().all() -def get_default_scenario_ids_for_portfolio(portfolio_id: int) -> List[int]: - # This should in reality always return exactly 1 ID, but there's currently - # no database constraint to enforce that, so account for 0 or >1 - stmt = select(ScenarioModel.id).where( +def get_default_plans_and_scenarios( + portfolio_id: int, +) -> Tuple[List[PlanModel], List[ScenarioModel]]: + plan_stmt = select(PlanModel).where( + (PlanModel.portfolio_id == portfolio_id) & (PlanModel.is_default == True) + ) + scenario_stmt = select(ScenarioModel).where( (ScenarioModel.portfolio_id == portfolio_id) & (ScenarioModel.is_default == True) ) + with db_read_session() as session: session_any: Any = session # Typehint as Any to satisfy Pylance... - return session_any.exec(stmt).scalars().all() - - -def set_plan_and_scenario_default(plan_id: int, default: bool) -> bool: - with db_session() as session: - plan: PlanModel = session.get(PlanModel, plan_id) - if not plan: - return False - - scenario_id = plan.scenario_id - - plan_mapper: Mapper[Any] = inspect(PlanModel) - scenario_mapper: Mapper[Any] = inspect(ScenarioModel) - - plan_mappings: List[Dict[str, Any]] = [{"id": plan.id, "is_default": default}] - scenario_mappings: List[Dict[str, Any]] = [ - {"id": scenario_id, "is_default": default} - ] - - session.bulk_update_mappings(plan_mapper, plan_mappings) - session.bulk_update_mappings(scenario_mapper, scenario_mappings) - session.commit() - - return True + plans: List[PlanModel] = session_any.exec(plan_stmt).scalars().all() + scenarios: List[ScenarioModel] = session_any.exec(scenario_stmt).scalars().all() + return (plans, scenarios) def bulk_update_plans( diff --git a/backend/categorisation/processor.py b/backend/categorisation/processor.py index ea12bc3b..2e4bab12 100644 --- a/backend/categorisation/processor.py +++ b/backend/categorisation/processor.py @@ -1,14 +1,12 @@ from collections import defaultdict -from typing import Dict, List, Optional, Tuple +from typing import Dict, List, Optional from backend.app.db.functions.recommendations_functions import ( bulk_update_plans, - get_default_scenario_ids_for_portfolio, - get_most_recent_plan_ids_by_scenario_ids, - get_plans_by_portfolio_id, - get_plans_by_scenario_ids, + get_default_plans_and_scenarios, + get_most_recent_plans_by_portfolio_id, + get_most_recent_plans_by_scenario_ids, get_scenarios_by_portfolio_id, - set_plan_and_scenario_default, ) from backend.app.db.models.recommendations import PlanModel, ScenarioModel from backend.app.domain.classes.plan import Plan @@ -22,29 +20,38 @@ def process_portfolio( portfolio_id: int, scenarios_to_consider: Optional[List[int]] = None, scenario_priority_order: Optional[List[int]] = None, -) -> None: +) -> None: # TODO: make this a class logger.info(f"Processing portfolio {portfolio_id}") + plans_by_id: Dict[int, Plan] = {} # TODO: make this an in-memory repository class + if scenarios_to_consider: if len(scenarios_to_consider) < 2: raise ValueError( "Cannot run auto categorisation for fewer than 2 scenarios" ) - if scenarios_to_consider is not None: - _unset_defaults_for_scenarios_not_being_considered( - portfolio_id, scenarios_to_consider - ) + # first get all plans that we're interested in + plans_for_consideration: List[Plan] = _load_plans_for_portfolio( + portfolio_id, scenarios_to_consider + ) + for plan in plans_for_consideration: + if plan.id is not None: # just in case + plans_by_id[plan.id] = plan - plans: List[Plan] = _load_plans_for_portfolio(portfolio_id, scenarios_to_consider) + # then unset existing defaults on domain objects regardless of whether they're under consideration or not + default_plans: List[Plan] = _get_default_plans(portfolio_id) + for plan in default_plans: + plan.set_default(False) + if plan.id is not None: # just in case + plans_by_id[plan.id] = plan - plans_by_property: Dict[int, List[Plan]] = _group_plans_by_property(plans) - - updated_plan_models: List[PlanModel] = [] - updated_scenario_models: List[ScenarioModel] = [] - - for property_id, property_plans in plans_by_property.items(): + # then set new defaults on domain objects under consideration + plans_for_consideration_by_property: Dict[int, List[Plan]] = ( + _group_plans_by_property(plans_for_consideration) + ) + for property_id, property_plans in plans_for_consideration_by_property.items(): if not property_plans: raise ValueError(f"No plans for property {property_id}") @@ -56,17 +63,13 @@ def process_portfolio( logger.error(f"Failed to find cheapest plan for property {property_id}") raise - updated_property_plan_models, updated_property_scenario_models = ( - _update_plan_and_scenario_objects(property_plans, cheapest_plan) - ) + property_plans = _update_plan_objects(property_plans, cheapest_plan) + for plan in property_plans: + if plan.id is not None: # just in case + plans_by_id[plan.id] = plan - updated_plan_models.extend(updated_property_plan_models) - updated_scenario_models.extend(updated_property_scenario_models) - - if len(updated_plan_models) > 0: - logger.info(f"Updating {len(updated_plan_models)} Plans in database") - bulk_update_plans(updated_plan_models, updated_scenario_models) - logger.info("Successfully updated Plan default values in database") + # then pass all domain objects to database to update (regardless of whether they've changed) + _update_plans_in_db(list(plans_by_id.values())) def choose_cheapest_relevant_plan( @@ -100,29 +103,17 @@ def choose_cheapest_relevant_plan( return cheapest_plans[0] -def _unset_defaults_for_scenarios_not_being_considered( - portfolio_id: int, scenarios_to_consider: List[int] -) -> None: - default_scenario_ids: List[int] = get_default_scenario_ids_for_portfolio( +def _get_default_plans(portfolio_id: int) -> List[Plan]: + default_plan_models, default_scenario_models = get_default_plans_and_scenarios( portfolio_id ) - scenarios_to_unset_default: List[int] = [] - for id in default_scenario_ids: - if id not in scenarios_to_consider: - scenarios_to_unset_default.append(id) - - if len(scenarios_to_unset_default) > 0: - logger.info( - f"Unsetting {scenarios_to_unset_default} as default scenario(s) as not included in provided list of scenarios to consider" + return [ + Plan.from_sqlalchemy( + p, next(s for s in default_scenario_models if s.id == p.scenario_id) ) - - if len(scenarios_to_unset_default) > 0: - plans_to_unset_default: List[int] = get_most_recent_plan_ids_by_scenario_ids( - scenarios_to_unset_default - ) - for plan_id in plans_to_unset_default: - set_plan_and_scenario_default(plan_id, False) # TODO: do this in batch + for p in default_plan_models + ] def _load_plans_for_portfolio( @@ -131,13 +122,17 @@ def _load_plans_for_portfolio( if scenarios_to_consider: logger.info(f"Getting plans for {len(scenarios_to_consider)} scenarios") - plan_models: List[PlanModel] = get_plans_by_scenario_ids(scenarios_to_consider) + plan_models: List[PlanModel] = get_most_recent_plans_by_scenario_ids( + scenarios_to_consider + ) logger.info(f"Got {len(plan_models)} plan models from database") else: logger.info( f"No list of Plans to consider provided. Getting all Plans for portfolio {portfolio_id}" ) - plan_models: List[PlanModel] = get_plans_by_portfolio_id(portfolio_id) + plan_models: List[PlanModel] = get_most_recent_plans_by_portfolio_id( + portfolio_id + ) plans: List[Plan] = [] @@ -170,26 +165,26 @@ def _group_plans_by_property(plans: List[Plan]) -> Dict[int, List[Plan]]: return grouped -def _update_plan_and_scenario_objects( - plans: List[Plan], cheapest_plan: Plan -) -> Tuple[List[PlanModel], List[ScenarioModel]]: - plans_to_update: List[Plan] = [] - +def _update_plan_objects(plans: List[Plan], cheapest_plan: Plan) -> List[Plan]: for plan in plans: should_be_default: bool = plan.id == cheapest_plan.id - if plan.record.is_default != should_be_default: - logger.info( - f"Setting Plan {plan.id} (Scenario Name: {plan.scenario.record.name}) to is_default: {should_be_default}" - ) - plan.set_default(should_be_default) - plans_to_update.append(plan) + plan.set_default(should_be_default) + if should_be_default: + logger.debug( + f"Setting Plan {plan.id} (Scenario Name: {plan.scenario.record.name}) to default" + ) + + return plans + + +def _update_plans_in_db(plans: List[Plan]) -> None: plan_models: List[PlanModel] = [] scenario_models: List[ScenarioModel] = [] - for plan in plans_to_update: + for plan in plans: plan_model, scenario_model = plan.to_sqlalchemy() plan_models.append(plan_model) scenario_models.append(scenario_model) - return (plan_models, scenario_models) + bulk_update_plans(plan_models, scenario_models) From 325e7f65e0df8a1fb7704bc828caa7e3c098412c Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Fri, 20 Feb 2026 16:13:30 +0000 Subject: [PATCH 33/46] make sure Plan object is instantiated correctly. Additional logging --- backend/categorisation/processor.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/backend/categorisation/processor.py b/backend/categorisation/processor.py index 2e4bab12..95d4de3a 100644 --- a/backend/categorisation/processor.py +++ b/backend/categorisation/processor.py @@ -46,6 +46,8 @@ def process_portfolio( if plan.id is not None: # just in case plans_by_id[plan.id] = plan + logger.info(f"Successfully unset {len(default_plans)} default plan(s)") + # then set new defaults on domain objects under consideration plans_for_consideration_by_property: Dict[int, List[Plan]] = ( _group_plans_by_property(plans_for_consideration) @@ -68,8 +70,11 @@ def process_portfolio( if plan.id is not None: # just in case plans_by_id[plan.id] = plan + logger.info("Successfully set defaults on Plan objects in memory") + # then pass all domain objects to database to update (regardless of whether they've changed) _update_plans_in_db(list(plans_by_id.values())) + logger.info(f"Successfully updated {len(plans_by_id)} Plans in database") def choose_cheapest_relevant_plan( @@ -110,7 +115,12 @@ def _get_default_plans(portfolio_id: int) -> List[Plan]: return [ Plan.from_sqlalchemy( - p, next(s for s in default_scenario_models if s.id == p.scenario_id) + p, + next( + Scenario.from_sqlalchemy(s) + for s in default_scenario_models + if s.id == p.scenario_id + ), ) for p in default_plan_models ] From 31cfa47d8feb90d5aaab0cee91d2ffc214e5db2b Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Fri, 20 Feb 2026 17:10:53 +0000 Subject: [PATCH 34/46] dont worry about default scenarios --- .../db/functions/recommendations_functions.py | 11 ++--- backend/categorisation/processor.py | 47 +++++++++---------- 2 files changed, 26 insertions(+), 32 deletions(-) diff --git a/backend/app/db/functions/recommendations_functions.py b/backend/app/db/functions/recommendations_functions.py index 141ba2dd..09d6da83 100644 --- a/backend/app/db/functions/recommendations_functions.py +++ b/backend/app/db/functions/recommendations_functions.py @@ -673,22 +673,17 @@ def get_scenarios_by_portfolio_id(portfolio_id: int) -> List[ScenarioModel]: return session_any.exec(stmt).scalars().all() -def get_default_plans_and_scenarios( +def get_default_plans( portfolio_id: int, -) -> Tuple[List[PlanModel], List[ScenarioModel]]: +) -> List[PlanModel]: plan_stmt = select(PlanModel).where( (PlanModel.portfolio_id == portfolio_id) & (PlanModel.is_default == True) ) - scenario_stmt = select(ScenarioModel).where( - (ScenarioModel.portfolio_id == portfolio_id) - & (ScenarioModel.is_default == True) - ) with db_read_session() as session: session_any: Any = session # Typehint as Any to satisfy Pylance... plans: List[PlanModel] = session_any.exec(plan_stmt).scalars().all() - scenarios: List[ScenarioModel] = session_any.exec(scenario_stmt).scalars().all() - return (plans, scenarios) + return plans def bulk_update_plans( diff --git a/backend/categorisation/processor.py b/backend/categorisation/processor.py index 95d4de3a..09db2983 100644 --- a/backend/categorisation/processor.py +++ b/backend/categorisation/processor.py @@ -3,7 +3,7 @@ from typing import Dict, List, Optional from backend.app.db.functions.recommendations_functions import ( bulk_update_plans, - get_default_plans_and_scenarios, + get_default_plans, get_most_recent_plans_by_portfolio_id, get_most_recent_plans_by_scenario_ids, get_scenarios_by_portfolio_id, @@ -23,6 +23,7 @@ def process_portfolio( ) -> None: # TODO: make this a class logger.info(f"Processing portfolio {portfolio_id}") + all_scenarios: List[Scenario] = _load_scenarios_for_portfolio(portfolio_id) plans_by_id: Dict[int, Plan] = {} # TODO: make this an in-memory repository class if scenarios_to_consider: @@ -33,14 +34,14 @@ def process_portfolio( # first get all plans that we're interested in plans_for_consideration: List[Plan] = _load_plans_for_portfolio( - portfolio_id, scenarios_to_consider + portfolio_id, all_scenarios, scenarios_to_consider ) for plan in plans_for_consideration: if plan.id is not None: # just in case plans_by_id[plan.id] = plan # then unset existing defaults on domain objects regardless of whether they're under consideration or not - default_plans: List[Plan] = _get_default_plans(portfolio_id) + default_plans: List[Plan] = _get_default_plans(portfolio_id, all_scenarios) for plan in default_plans: plan.set_default(False) if plan.id is not None: # just in case @@ -108,26 +109,28 @@ def choose_cheapest_relevant_plan( return cheapest_plans[0] -def _get_default_plans(portfolio_id: int) -> List[Plan]: - default_plan_models, default_scenario_models = get_default_plans_and_scenarios( - portfolio_id - ) +def _get_default_plans(portfolio_id: int, scenarios: List[Scenario]) -> List[Plan]: + default_plan_models = get_default_plans(portfolio_id) + + scenario_map = {s.id: s for s in scenarios} return [ - Plan.from_sqlalchemy( - p, - next( - Scenario.from_sqlalchemy(s) - for s in default_scenario_models - if s.id == p.scenario_id - ), - ) + Plan.from_sqlalchemy(p, scenario_map[p.scenario_id]) for p in default_plan_models + if p.scenario_id in scenario_map ] +def _load_scenarios_for_portfolio(portfolio_id: int) -> List[Scenario]: + scenario_models: List[ScenarioModel] = get_scenarios_by_portfolio_id(portfolio_id) + + return [Scenario.from_sqlalchemy(s) for s in scenario_models] + + def _load_plans_for_portfolio( - portfolio_id: int, scenarios_to_consider: Optional[List[int]] = None + portfolio_id: int, + all_scenarios: List[Scenario], + scenarios_to_consider: Optional[List[int]] = None, ) -> List[Plan]: if scenarios_to_consider: @@ -146,21 +149,17 @@ def _load_plans_for_portfolio( plans: List[Plan] = [] - scenarios: List[ScenarioModel] = get_scenarios_by_portfolio_id(portfolio_id) - - if not scenarios: + if not all_scenarios: raise Exception(f"No scenarios found for Portfolio {portfolio_id}") for model in plan_models: - scenario_model = next((s for s in scenarios if s.id == model.scenario_id)) - if not scenario_model: + scenario = next((s for s in all_scenarios if s.id == model.scenario_id)) + if not scenario: logger.info(f"No Scenario associated with Plan of ID {model.id}") continue - plans.append( - Plan.from_sqlalchemy(model, Scenario.from_sqlalchemy(scenario_model)) - ) + plans.append(Plan.from_sqlalchemy(model, scenario)) logger.info(f"Got {len(plans)} Plans") return plans From 5305643991e4986fd077c47d125cdb120ce3ff61 Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Mon, 23 Feb 2026 12:44:42 +0000 Subject: [PATCH 35/46] pass needs ventilation to optimiser functon' --- backend/engine/engine.py | 10 +++++++--- .../optimiser/optimiser_functions.py | 17 ++++++++++------- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/backend/engine/engine.py b/backend/engine/engine.py index 80d6d078..6c6b0c70 100644 --- a/backend/engine/engine.py +++ b/backend/engine/engine.py @@ -1053,7 +1053,9 @@ async def model_engine(body: PlanTriggerRequest): property_required_measures = [m for m in recommendations[p.id] if m[0]["type"] in body.required_measures] measures_to_optimise = [m for m in recommendations[p.id] if m[0]["type"] not in body.required_measures] - ventilation_included = "ventilation" in property_measure_types + ventilation_included = ( + "ventilation" in property_measure_types or "mechanical_ventilation" in property_measure_types + ) # If a measure requiring ventilation is selected, and the property does not have ventilation, we enfore # its inclusion @@ -1177,8 +1179,10 @@ async def model_engine(body: PlanTriggerRequest): recommendations=recommendations, selected=selected, ) - # Add best practice measures (ventilation/trickle vents) - selected = optimiser_functions.add_best_practice_measures(p.id, solution, recommendations, selected) + # Add best practice measures (ventilation/trickle vents) - pass needs_ventilation flag + selected = optimiser_functions.add_best_practice_measures( + p.id, solution, recommendations, selected, needs_ventilation + ) # Final flattening - we pass what the battery SAP score would be, regardless if the battery was selected recommendations[p.id] = optimiser_functions.flatten_recommendations_with_defaults( p.id, recommendations, selected, battery_sap_score diff --git a/recommendations/optimiser/optimiser_functions.py b/recommendations/optimiser/optimiser_functions.py index d704b3fb..e916f0fd 100644 --- a/recommendations/optimiser/optimiser_functions.py +++ b/recommendations/optimiser/optimiser_functions.py @@ -1,4 +1,5 @@ import pandas as pd +from typing import List, Dict, Any, Set import backend.app.assumptions as assumptions from backend.Property import Property from backend.app.plan.schemas import PlanTriggerRequest @@ -300,7 +301,13 @@ def add_required_measures(property_id, property_required_measures, recommendatio ] -def add_best_practice_measures(property_id, solution, recommendations, selected): +def add_best_practice_measures( + property_id: int, + solution: List[Dict[str, Any]], + recommendations: Dict[int, List[List[Dict[str, Any]]]], + selected: Set[str], + needs_ventilation: bool +): """ Ensures best-practice measures like ventilation and trickle vents are included in the selected recommendations when appropriate. @@ -320,6 +327,8 @@ def add_best_practice_measures(property_id, solution, recommendations, selected) All recommendations for all properties, keyed by property id. selected : set Set of already selected recommendation IDs. + needs_ventilation : bool + Whether the property requires mechanical ventilation to accompany certain measures. Returns ------- @@ -329,12 +338,6 @@ def add_best_practice_measures(property_id, solution, recommendations, selected) # Check if any selected measure requires ventilation ventilation_selected = [r for r in solution if "+mechanical_ventilation" in r["type"]] - # If ventilation has been selected, or one of the measures needs ventilation, we need to ensure ventilation is - # included - needs_ventilation = any( - x in [r["type"] for r in solution] for x in assumptions.measures_needing_ventilation - ) or len(ventilation_selected) > 0 - if needs_ventilation: ventilation_rec = next( (r[0] for r in recommendations[property_id] if r[0]["type"] == "mechanical_ventilation"), From c514ef53dc0bf1880e75c1586e0e45bac1d57d5e Mon Sep 17 00:00:00 2001 From: Daniel Roth Date: Mon, 23 Feb 2026 13:03:03 +0000 Subject: [PATCH 36/46] Add name to plan record and include in mapping --- backend/app/domain/classes/plan.py | 2 ++ backend/app/domain/records/plan_record.py | 1 + 2 files changed, 3 insertions(+) diff --git a/backend/app/domain/classes/plan.py b/backend/app/domain/classes/plan.py index 351ea512..e7455427 100644 --- a/backend/app/domain/classes/plan.py +++ b/backend/app/domain/classes/plan.py @@ -47,6 +47,7 @@ class Plan: valuation_increase=plan_model.valuation_increase, cost_of_works=plan_model.cost_of_works, contingency_cost=plan_model.contingency_cost, + name=plan_model.name, ) return cls(record=record, scenario=scenario, id=plan_model.id) @@ -137,6 +138,7 @@ class Plan: valuation_increase=record.valuation_increase, cost_of_works=record.cost_of_works, contingency_cost=record.contingency_cost, + name=record.name, ) return PlanPersistence(plan=plan_model, scenario=scenario_model) diff --git a/backend/app/domain/records/plan_record.py b/backend/app/domain/records/plan_record.py index 2df7a7c6..63a82993 100644 --- a/backend/app/domain/records/plan_record.py +++ b/backend/app/domain/records/plan_record.py @@ -29,3 +29,4 @@ class PlanRecord: valuation_increase: Optional[float] = None cost_of_works: Optional[float] = None contingency_cost: Optional[float] = None + name: Optional[str] = None From 84aef797355146a2b5901b59adcfa6be3688fa95 Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Mon, 23 Feb 2026 13:35:31 +0000 Subject: [PATCH 37/46] Added tests for checking ventilation --- backend/engine/engine.py | 7 ++- .../optimiser/optimiser_functions.py | 23 +++++++ .../tests/test_optimiser_functions.py | 60 +++++++++++++++++++ 3 files changed, 87 insertions(+), 3 deletions(-) diff --git a/backend/engine/engine.py b/backend/engine/engine.py index 6c6b0c70..dd0aebe4 100644 --- a/backend/engine/engine.py +++ b/backend/engine/engine.py @@ -1060,9 +1060,10 @@ async def model_engine(body: PlanTriggerRequest): # If a measure requiring ventilation is selected, and the property does not have ventilation, we enfore # its inclusion - needs_ventilation = any( - x in property_measure_types for x in assumptions.measures_needing_ventilation - ) and not p.has_ventilation and ventilation_included + needs_ventilation = optimiser_functions.check_needs_ventilation( + property_measure_types, assumptions.measures_needing_ventilation, p.has_ventilation, + ventilation_included + ) if not measures_to_optimise: # Nothing to do, we just reshape the recommendations diff --git a/recommendations/optimiser/optimiser_functions.py b/recommendations/optimiser/optimiser_functions.py index e916f0fd..c17cdf1e 100644 --- a/recommendations/optimiser/optimiser_functions.py +++ b/recommendations/optimiser/optimiser_functions.py @@ -398,3 +398,26 @@ def flatten_recommendations_with_defaults(property_id, recommendations, selected # Flatten the nested list of lists into a single list return [rec for recommendations_by_type in final_recommendations for rec in recommendations_by_type] + + +def check_needs_ventilation( + property_measure_types: Set[str], + measures_needing_ventilation: List[str], + has_ventilation: bool, + ventilation_included: bool +) -> bool: + """ + Function to check if we need to include ventilation based on the measures selected and the property + features + :param property_measure_types: The set of measure types recommended for the property + :param measures_needing_ventilation: The set of measure types that require ventilation + :param has_ventilation: Whether the property currently has ventilation + :param ventilation_included: Whether ventilation is already included in the recommended measures + :return: Boolean indicating whether ventilation needs to be included in the recommendations + + # TODO - none of the inputs of this function are well structured and so this is quite brittle - we should + consider refactoring to make this more robust + """ + return any( + x in property_measure_types for x in measures_needing_ventilation + ) and not has_ventilation and ventilation_included diff --git a/recommendations/tests/test_optimiser_functions.py b/recommendations/tests/test_optimiser_functions.py index f0ca6dac..8f898970 100644 --- a/recommendations/tests/test_optimiser_functions.py +++ b/recommendations/tests/test_optimiser_functions.py @@ -510,3 +510,63 @@ class TestStrategicOptimiser: assert opt.strategy_used.value == "case_2_solve_max_gain_under_budget" assert opt.solution_cost == 7787.068 assert opt.solution_gain == 28.8 + + +class TestCheckNeedsVentilation: + + def measure_types_includes_ventilation_no_existing_ventilation(self): + property_measure_types = {'mechanical_ventilation', 'cavity_wall_insulation', 'suspended_floor_insulation', + 'secondary_heating', 'loft_insulation', 'heating', 'low_energy_lighting'} + + measures_needing_ventilation = ['internal_wall_insulation', 'external_wall_insulation', + 'cavity_wall_insulation'] + + has_ventilation = False + + ventilation_included = True + + result = optimiser_functions.check_needs_ventilation( + property_measure_types, measures_needing_ventilation, has_ventilation, + ventilation_included + ) + + assert result == True + + def measure_types_includes_ventilation_existing_ventilation(self): + property_measure_types = {'mechanical_ventilation', 'cavity_wall_insulation', 'suspended_floor_insulation', + 'secondary_heating', 'loft_insulation', 'heating', 'low_energy_lighting'} + + measures_needing_ventilation = ['internal_wall_insulation', 'external_wall_insulation', + 'cavity_wall_insulation'] + + has_ventilation = True + + ventilation_included = True + + result = optimiser_functions.check_needs_ventilation( + property_measure_types, measures_needing_ventilation, has_ventilation, + ventilation_included + ) + + assert result == False + + def measure_types_includes_ventilation_existing_ventilation(self): + property_measure_types_without_ventilation = { + 'cavity_wall_insulation', 'suspended_floor_insulation', + 'secondary_heating', 'loft_insulation', 'heating', + 'low_energy_lighting' + } + + measures_needing_ventilation = ['internal_wall_insulation', 'external_wall_insulation', + 'cavity_wall_insulation'] + + has_ventilation = False + + ventilation_included = True + + result = optimiser_functions.check_needs_ventilation( + property_measure_types_without_ventilation, measures_needing_ventilation, has_ventilation, + ventilation_included + ) + + assert result == False From 73928c67c587ce841ea7eb684a9767eed46b3b41 Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Mon, 23 Feb 2026 13:37:17 +0000 Subject: [PATCH 38/46] added future todo for measure types --- backend/engine/engine.py | 1 + 1 file changed, 1 insertion(+) diff --git a/backend/engine/engine.py b/backend/engine/engine.py index dd0aebe4..101f6ada 100644 --- a/backend/engine/engine.py +++ b/backend/engine/engine.py @@ -1053,6 +1053,7 @@ async def model_engine(body: PlanTriggerRequest): property_required_measures = [m for m in recommendations[p.id] if m[0]["type"] in body.required_measures] measures_to_optimise = [m for m in recommendations[p.id] if m[0]["type"] not in body.required_measures] + # TODO - formalise property measure types into an enum ventilation_included = ( "ventilation" in property_measure_types or "mechanical_ventilation" in property_measure_types ) From 694717bd34a5e9aedaeca942abbf9905fcb81e2d Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Mon, 23 Feb 2026 14:14:07 +0000 Subject: [PATCH 39/46] addressing Dan's feedback --- recommendations/optimiser/optimiser_functions.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/recommendations/optimiser/optimiser_functions.py b/recommendations/optimiser/optimiser_functions.py index c17cdf1e..ab98113c 100644 --- a/recommendations/optimiser/optimiser_functions.py +++ b/recommendations/optimiser/optimiser_functions.py @@ -403,21 +403,25 @@ def flatten_recommendations_with_defaults(property_id, recommendations, selected def check_needs_ventilation( property_measure_types: Set[str], measures_needing_ventilation: List[str], - has_ventilation: bool, - ventilation_included: bool + property_already_has_ventilation: bool, + ventilation_in_included_measures: bool ) -> bool: """ Function to check if we need to include ventilation based on the measures selected and the property features :param property_measure_types: The set of measure types recommended for the property :param measures_needing_ventilation: The set of measure types that require ventilation - :param has_ventilation: Whether the property currently has ventilation - :param ventilation_included: Whether ventilation is already included in the recommended measures + :param property_already_has_ventilation: Whether the property currently has ventilation + :param ventilation_in_included_measures: Whether ventilation is already included in the recommended + measures :return: Boolean indicating whether ventilation needs to be included in the recommendations # TODO - none of the inputs of this function are well structured and so this is quite brittle - we should consider refactoring to make this more robust """ - return any( + + needs_ventilation = any( x in property_measure_types for x in measures_needing_ventilation - ) and not has_ventilation and ventilation_included + ) + + return needs_ventilation and not has_ventilation and ventilation_included From 9dda6fb434c4c13306924f7be16a17d82bdd0ddb Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Mon, 23 Feb 2026 15:33:32 +0000 Subject: [PATCH 40/46] fixed test and variable renames in function --- recommendations/optimiser/optimiser_functions.py | 2 +- recommendations/tests/test_optimiser_functions.py | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/recommendations/optimiser/optimiser_functions.py b/recommendations/optimiser/optimiser_functions.py index ab98113c..4b0d4b94 100644 --- a/recommendations/optimiser/optimiser_functions.py +++ b/recommendations/optimiser/optimiser_functions.py @@ -424,4 +424,4 @@ def check_needs_ventilation( x in property_measure_types for x in measures_needing_ventilation ) - return needs_ventilation and not has_ventilation and ventilation_included + return needs_ventilation and not property_already_has_ventilation and ventilation_in_included_measures diff --git a/recommendations/tests/test_optimiser_functions.py b/recommendations/tests/test_optimiser_functions.py index 8f898970..debd2d88 100644 --- a/recommendations/tests/test_optimiser_functions.py +++ b/recommendations/tests/test_optimiser_functions.py @@ -143,7 +143,9 @@ class TestAddBestPracticeMeasures: ] } selected = set() - updated = optimiser_functions.add_best_practice_measures(property_id, solution, recommendations, selected) + updated = optimiser_functions.add_best_practice_measures( + property_id, solution, recommendations, selected, True + ) assert "vent1" in updated assert "trickle1" in updated @@ -273,7 +275,7 @@ class TestIncreasingEpcE2e: total_optimised_gain = sum(m["gain"] for m in solution) assert total_optimised_gain == 17.6, "Total gain of optimised measures should meet or exceed target gain" - selected = optimiser_functions.add_best_practice_measures(p.id, solution, recommendations, selected) + selected = optimiser_functions.add_best_practice_measures(p.id, solution, recommendations, selected, False) # Flatten recommendations for output flattened = optimiser_functions.flatten_recommendations_with_defaults(p.id, recommendations, selected) From 40f3c36dbb78922c689b8be05dbfdba827c28e2b Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Mon, 23 Feb 2026 23:26:41 +0000 Subject: [PATCH 41/46] adding further tests for filtering phase adjustments --- backend/Property.py | 2 +- recommendations/Recommendations.py | 40 +++- recommendations/SecondaryHeating.py | 3 + recommendations/tests/test_recommendations.py | 194 +++++++++++++++++- 4 files changed, 228 insertions(+), 11 deletions(-) diff --git a/backend/Property.py b/backend/Property.py index 6a84fc09..f196f49b 100644 --- a/backend/Property.py +++ b/backend/Property.py @@ -490,7 +490,7 @@ class Property: for rec_id in rec_ids: sim_epc = self.simulation_epcs[rec_id].copy() rec_impact = [x for x in impact_summary if x["recommendation_id"] == rec_id][0] - # We update all of the features that should have an impact on the kwh model + # We update all features that should have an impact on the kwh model sim_epc.update( { diff --git a/recommendations/Recommendations.py b/recommendations/Recommendations.py index acd49e05..5525b7a0 100644 --- a/recommendations/Recommendations.py +++ b/recommendations/Recommendations.py @@ -499,8 +499,16 @@ class Recommendations: return predicted_appliances_cost_reduction, predicted_appliances_kwh_reduction @staticmethod - def _check_ventilation_out_of_bounds(sap_impact, ventilation_sap_limit): - return (sap_impact < ventilation_sap_limit) or (sap_impact >= 0) + def _check_ventilation_out_of_bounds(sap_impact: float, ventilation_sap_limit: float) -> bool: + """ + Checks if the SAP impact of a ventilation recommendation is out of bounds, which would indicate that the + recommendation is not appropriate. + :param sap_impact: The SAP impact of the ventilation recommendation, which is typically negative or zero + :param ventilation_sap_limit: The SAP limit for ventilation recommendations, which is typically a negative + number. E.g. -4 + :return: + """ + return (sap_impact < ventilation_sap_limit) or (sap_impact > 0) @staticmethod def _adjust_ventilation_sap(sap_impact, ventilation_sap_limit): @@ -691,7 +699,8 @@ class Recommendations: previous_phase_values: dict, current_phase_values: dict, adjustments: list, - property_instance, + property_instance: Property, + model_predicted_sap: float, ): # For the moment, we cap the number of SAP points that can be achieved by LEDs at 2 if rec["type"] == "low_energy_lighting": @@ -785,7 +794,6 @@ class Recommendations: # Update the current phase values current_phase_values["sap"] = previous_phase_values["sap"] + property_phase_impact["sap"] - elif rec["type"] == "loft_insulation": # When we have a loft insulation recommendation, where there is an extension and the existing # amount of loft insulation is already good, we limit the SAP points @@ -831,6 +839,27 @@ class Recommendations: # Update the current phase values current_phase_values["sap"] = previous_phase_values["sap"] + property_phase_impact["sap"] + elif rec["measure_type"] in ["roomstat_programmer_trvs", "time_temperature_zone_control"]: + # We trim the SAP point recommendations based on the minimum of the predicted and the survey SAP + # points + predicted_difference = model_predicted_sap - previous_phase_values["sap_prediction"] + proposed_impact = property_phase_impact["sap"] + numerically_the_same = np.isclose(proposed_impact, predicted_difference) + + if predicted_difference > 0 and (predicted_difference < proposed_impact) and not numerically_the_same: + # We constrain the impact based on what the model predicts. + # We update the proposed impact to be the predicted difference + adjustments.append( + { + "recommendation_id": rec["recommendation_id"], + "phase": rec["phase"], + # If we've made an adjustment, it will be negative + "sap_adjustment": property_phase_impact["sap"] - predicted_difference, + } + ) + property_phase_impact["sap"] = predicted_difference + # Update the current phase values + current_phase_values["sap"] = previous_phase_values["sap"] + property_phase_impact["sap"] return property_phase_impact, current_phase_values, adjustments @@ -963,7 +992,8 @@ class Recommendations: previous_phase_values=previous_phase_values, current_phase_values=current_phase_values, adjustments=adjustments, - property_instance=property_instance + property_instance=property_instance, + model_predicted_sap=phase_energy_efficiency_metrics["sap_change"], ) # Insert this information into the recommendation. diff --git a/recommendations/SecondaryHeating.py b/recommendations/SecondaryHeating.py index ee7eae1c..ef0fc2d2 100644 --- a/recommendations/SecondaryHeating.py +++ b/recommendations/SecondaryHeating.py @@ -18,6 +18,9 @@ class SecondaryHeating: def recommend(self, phase: int): # Reset self.recommendation = [] + if self.property.epc_record.secondheat_description in ["None", None]: + # No secondary heating system, so no recommendation to remove it + return if self.property.data['number-habitable-rooms'] > self.property.data['number-heated-rooms']: n_rooms = self.property.data['number-habitable-rooms'] - self.property.data['number-heated-rooms'] diff --git a/recommendations/tests/test_recommendations.py b/recommendations/tests/test_recommendations.py index e3bcbb2f..747b0b2e 100644 --- a/recommendations/tests/test_recommendations.py +++ b/recommendations/tests/test_recommendations.py @@ -373,7 +373,7 @@ def test_filter_phase_adjustment(input_data, expected): "sap_impact, limit, expected", [ (1.0, -4, True), # positive SAP not allowed - (0.0, -4, True), # zero not allowed + (0.0, -4, False), # zero is allowed (-1.0, -4, False), # valid range (-3.9, -4, False), # valid range (-4.0, -4, False), # exact lower bound allowed @@ -1476,7 +1476,9 @@ def test_lighting_and_loft_adjustment_combined(property_instance, heat_demand_pr assert adjustments2 == [ {'recommendation_id': '0_phase=0', 'phase': 0, 'sap_adjustment': np.float64(1.7)}, - {'recommendation_id': '4_phase=2', 'phase': 2, 'sap_adjustment': np.float64(4.0)} + {'recommendation_id': '4_phase=2', 'phase': 2, 'sap_adjustment': np.float64(4.0)}, + {'recommendation_id': '5_phase=3', 'phase': 3, 'sap_adjustment': np.float64(1.0)}, + {'recommendation_id': '6_phase=3', 'phase': 3, 'sap_adjustment': np.float64(1.0000000000000027)} ] @@ -1499,7 +1501,8 @@ def test_mechanical_ventilation_sap_floor(property_instance): previous_phase_values=previous_phase_values, current_phase_values=current_phase_values, adjustments=adjustments, - property_instance=property_instance + property_instance=property_instance, + model_predicted_sap=0 ) ) @@ -1538,7 +1541,8 @@ def test_mechanical_ventilation_no_floor_adjustment(property_instance): previous_phase_values=previous_phase_values, current_phase_values=current_phase_values, adjustments=adjustments, - property_instance=property_instance + property_instance=property_instance, + model_predicted_sap=0 ) ) @@ -1570,7 +1574,8 @@ def test_mechanical_ventilation_exactly_one_no_adjustment(property_instance): previous_phase_values=previous_phase_values, current_phase_values=current_phase_values, adjustments=adjustments, - property_instance=property_instance + property_instance=property_instance, + model_predicted_sap=0 ) ) @@ -1578,3 +1583,182 @@ def test_mechanical_ventilation_exactly_one_no_adjustment(property_instance): assert updated_adjustments == [] assert updated_current["sap"] == 1.0 assert updated_impact["sap"] == -1.0 + + +def test_mechanical_ventilation_sap_zero_no_adjustment(property_instance): + # Test when SAP = 0 + rec = { + "type": "mechanical_ventilation", + "recommendation_id": "mv_test", + "phase": 1, + } + + previous_phase_values = {'phase': 0, 'representative': True, 'recommendation_id': '0_phase=0', + 'measure_type': 'flat_roof_insulation', 'sap': 68.0, 'carbon': np.float64(0.5), + 'heat_demand': np.float64(300.1), 'sap_prediction': np.float64(71.7)} + current_phase_values = {'sap': 68.0, 'carbon': np.float64(0.5), 'heat_demand': np.float64(307.0)} + property_phase_impact = {'sap': 0, 'carbon': 0, 'heat_demand': np.float64(-6.899999999999977)} + adjustments = [] + + updated_impact, updated_current, updated_adjustments = ( + Recommendations._apply_measure_specific_rules( + rec=rec, + property_phase_impact=property_phase_impact, + previous_phase_values=previous_phase_values, + current_phase_values=current_phase_values, + adjustments=adjustments, + property_instance=property_instance, + model_predicted_sap=0 + ) + ) + + # SAP is already at 0 → no adjustment expected + assert updated_adjustments == [] + assert updated_current["sap"] == 68.0 + assert updated_impact["sap"] == 0 + + +def test_mv_valid_negative_no_adjustment(property_instance): + rec = {"type": "mechanical_ventilation", "recommendation_id": "mv", "phase": 1} + + previous = {"sap": 70.0} + current = {"sap": 67.0} + impact = {"sap": -3.0, "carbon": 0, "heat_demand": 0} + adjustments = [] + + updated_impact, updated_current, updated_adjustments = ( + Recommendations._apply_measure_specific_rules( + rec, impact, previous, current, adjustments, property_instance, 0 + ) + ) + + assert updated_adjustments == [] + assert updated_current["sap"] == 67.0 + assert updated_impact["sap"] == -3.0 + + +def test_mv_zero_impact_allowed(property_instance): + rec = {"type": "mechanical_ventilation", "recommendation_id": "mv", "phase": 1} + + previous = {"sap": 68.0, "sap_prediction": 71.7} + current = {"sap": 68.0} + impact = {"sap": 0.0, "carbon": 0, "heat_demand": 0} + adjustments = [] + + updated_impact, updated_current, updated_adjustments = ( + Recommendations._apply_measure_specific_rules( + rec, impact, previous, current, adjustments, property_instance, 0 + ) + ) + + assert updated_adjustments == [] + assert updated_current["sap"] == 68.0 + assert updated_impact["sap"] == 0.0 + + +def test_mv_positive_impact_corrected(property_instance): + rec = {"type": "mechanical_ventilation", "recommendation_id": "mv", "phase": 1} + + previous = {"sap": 60.0} + current = {"sap": 61.0} + impact = {"sap": 1.0, "carbon": 0, "heat_demand": 0} + adjustments = [] + + updated_impact, updated_current, updated_adjustments = ( + Recommendations._apply_measure_specific_rules( + rec, impact, previous, current, adjustments, property_instance, 0 + ) + ) + + assert len(updated_adjustments) == 1 + assert updated_current["sap"] == previous["sap"] + updated_impact["sap"] + assert updated_impact["sap"] <= 0 + + +def test_mv_below_lower_bound_corrected(property_instance): + rec = {"type": "mechanical_ventilation", "recommendation_id": "mv", "phase": 1} + + previous = {"sap": 70.0} + current = {"sap": 64.0} + impact = {"sap": -6.0, "carbon": 0, "heat_demand": 0} + adjustments = [] + + updated_impact, updated_current, updated_adjustments = ( + Recommendations._apply_measure_specific_rules( + rec, impact, previous, current, adjustments, property_instance, 0 + ) + ) + + assert len(updated_adjustments) == 1 + assert updated_impact["sap"] >= -4 + + +def test_mv_floor_triggered(property_instance): + rec = {"type": "mechanical_ventilation", "recommendation_id": "mv", "phase": 1} + + previous = {"sap": 2.0} + current = {"sap": 0.5} + impact = {"sap": -1.5, "carbon": 0, "heat_demand": 0} + adjustments = [] + + updated_impact, updated_current, updated_adjustments = ( + Recommendations._apply_measure_specific_rules( + rec, impact, previous, current, adjustments, property_instance, 0 + ) + ) + + assert updated_current["sap"] == 1.0 + assert updated_adjustments[0]["sap_adjustment"] > 0 + + +def test_mv_exactly_one_no_floor(property_instance): + rec = {"type": "mechanical_ventilation", "recommendation_id": "mv", "phase": 1} + + previous = {"sap": 2.0} + current = {"sap": 1.0} + impact = {"sap": -1.0, "carbon": 0, "heat_demand": 0} + adjustments = [] + + updated_impact, updated_current, updated_adjustments = ( + Recommendations._apply_measure_specific_rules( + rec, impact, previous, current, adjustments, property_instance, 0 + ) + ) + + assert updated_adjustments == [] + assert updated_current["sap"] == 1.0 + + +def test_lighting_no_cap(property_instance): + rec = {"type": "low_energy_lighting", "recommendation_id": "led", "phase": 1, + "co2_equivalent_savings": 0} + + previous = {"sap": 60.0, "carbon": 2.0} + current = {"sap": 61.0, "carbon": 2.0} + impact = {"sap": 1.0, "carbon": 0, "heat_demand": 0} + adjustments = [] + + updated_impact, updated_current, updated_adjustments = ( + Recommendations._apply_measure_specific_rules( + rec, impact, previous, current, adjustments, property_instance, 0 + ) + ) + + assert updated_adjustments == [] + + +def test_filter_phase_adjustments(): + example_adjustments = [ + {'recommendation_id': '0_phase=0', 'phase': 0, 'sap_adjustment': np.float64(1.7)}, + {'recommendation_id': '4_phase=2', 'phase': 2, 'sap_adjustment': np.float64(4.0)}, + {'recommendation_id': '5_phase=3', 'phase': 3, 'sap_adjustment': np.float64(1.0)}, + {'recommendation_id': '6_phase=3', 'phase': 3, 'sap_adjustment': np.float64(1.0000000000000027)} + ] + + res = Recommendations._filter_phase_adjustment(example_adjustments) + + assert res == [ + {'recommendation_id': '0_phase=0', 'phase': 0, 'sap_adjustment': np.float64(1.7)}, + {'recommendation_id': '4_phase=2', 'phase': 2, 'sap_adjustment': np.float64(4.0)}, + {'recommendation_id': '6_phase=3', 'phase': 3, 'sap_adjustment': np.float64(1.0000000000000027)} + ] From b14c81fa8334b38baee6c5540cbfdec808483ccb Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Tue, 24 Feb 2026 00:32:03 +0000 Subject: [PATCH 42/46] allow slightly negative impact on cost savings --- recommendations/optimiser/optimiser_functions.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/recommendations/optimiser/optimiser_functions.py b/recommendations/optimiser/optimiser_functions.py index 4b0d4b94..a5cbf90d 100644 --- a/recommendations/optimiser/optimiser_functions.py +++ b/recommendations/optimiser/optimiser_functions.py @@ -79,14 +79,14 @@ def prepare_input_measures( # if recs[0]["type"] == "solar_pv": # recs = [r for r in recs if ~r["has_battery"]] - # Only include measures with non-negative cost savings + # Only include measures with non-negative cost savings - we allow for a minor negative impact if eco_measures: recs_to_append = [ - rec for rec in recs if (rec["energy_cost_savings"] >= 0) or (rec["measure_type"] in eco_measures) + rec for rec in recs if (rec["energy_cost_savings"] >= -10) or (rec["measure_type"] in eco_measures) ] else: recs_to_append = [ - rec for rec in recs if (rec["energy_cost_savings"] >= 0) + rec for rec in recs if (rec["energy_cost_savings"] >= -10) ] if not recs_to_append: continue From de360ab8660761861b736742854b1ac8b677ece4 Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Tue, 24 Feb 2026 19:34:51 +0000 Subject: [PATCH 43/46] fixed issue when phase is 0 --- .idea/Model.iml | 2 +- .idea/misc.xml | 2 +- recommendations/Recommendations.py | 4 +++- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/.idea/Model.iml b/.idea/Model.iml index c6561970..e1ca1b70 100644 --- a/.idea/Model.iml +++ b/.idea/Model.iml @@ -7,7 +7,7 @@ - + \ No newline at end of file diff --git a/.idea/misc.xml b/.idea/misc.xml index 50cad4ca..b1ee5ffa 100644 --- a/.idea/misc.xml +++ b/.idea/misc.xml @@ -3,7 +3,7 @@ - + diff --git a/recommendations/Recommendations.py b/recommendations/Recommendations.py index 5525b7a0..80cc06b4 100644 --- a/recommendations/Recommendations.py +++ b/recommendations/Recommendations.py @@ -582,6 +582,7 @@ class Recommendations: if rec_phase == starting_phase: return { "sap": float(property_instance.data["current-energy-efficiency"]), + "sap_prediction": float(property_instance.data["current-energy-efficiency"]), "carbon": float(property_instance.data["co2-emissions-current"]), "heat_demand": float(property_instance.data["energy-consumption-current"]), } @@ -599,12 +600,13 @@ class Recommendations: if not previous_phase_reps: return { "sap": float(property_instance.data["current-energy-efficiency"]), + "sap_prediction": float(property_instance.data["current-energy-efficiency"]), "carbon": float(property_instance.data["co2-emissions-current"]), "heat_demand": float(property_instance.data["energy-consumption-current"]), } # Median fallback (including zero-length case) - keys = ("sap", "carbon", "heat_demand") + keys = ("sap", "sap_prediction", "carbon", "heat_demand") return { key: np.median([item[key] for item in previous_phase_reps]) for key in keys From 088ea1e1c2fec9a93baf238c8cd62f5fce78317d Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Tue, 24 Feb 2026 19:54:31 +0000 Subject: [PATCH 44/46] zero gain --- .../optimiser/funding_optimiser.py | 10 + .../tests/test_optimiser_functions.py | 276 +++++++++++++++++- recommendations/tests/test_recommendations.py | 7 +- 3 files changed, 287 insertions(+), 6 deletions(-) diff --git a/recommendations/optimiser/funding_optimiser.py b/recommendations/optimiser/funding_optimiser.py index 324e2c74..69a6bc48 100644 --- a/recommendations/optimiser/funding_optimiser.py +++ b/recommendations/optimiser/funding_optimiser.py @@ -655,6 +655,11 @@ def optimise_with_scenarios( 1) With air source heat pump AND required insulation """ + # Universally handle zero gain + if target_gain is not None: + if target_gain <= 0: + return pd.DataFrame([]) + solutions = [] paths = [] # Produce the unique list of measure types @@ -770,6 +775,11 @@ def optimise_with_scenarios( for fixed in fixed_selections: + if target_gain is not None: + if target_gain <= 0: + # If we don't have any gain, we don't actually need to do this + continue + # fixed = [(gi, oi, opt), ...] fixed_items = [opt for (_, _, opt) in fixed] fixed_groups = {gi for (gi, _, _) in fixed} diff --git a/recommendations/tests/test_optimiser_functions.py b/recommendations/tests/test_optimiser_functions.py index debd2d88..08541c21 100644 --- a/recommendations/tests/test_optimiser_functions.py +++ b/recommendations/tests/test_optimiser_functions.py @@ -3,9 +3,19 @@ import numpy as np from types import SimpleNamespace from recommendations.tests.test_data.measures_to_optimise import measures_to_optimise from recommendations.optimiser import optimiser_functions +from recommendations.optimiser.funding_optimiser import optimise_with_scenarios from recommendations.optimiser.GainOptimiser import GainOptimiser from recommendations.optimiser.CostOptimiser import CostOptimiser -from recommendations.optimiser.StrategicOptimiser import StrategicOptimiser, Strategies +from recommendations.optimiser.StrategicOptimiser import StrategicOptimiser + + +@pytest.fixture +def property_instance(): + return SimpleNamespace( + id="P1", + has_ventilation=False, + data={"current-energy-efficiency": "52"}, + ) class TestPrepareInputMeasures: @@ -48,8 +58,9 @@ class TestPrepareInputMeasures: def test_filters_out_negative_cost_savings(self): recs = [ [{"recommendation_id": "bad1", "type": "loft_insulation", "total": 200, "kwh_savings": 100, - "energy_cost_savings": -5, "has_battery": False, - "partial_project_funding": 0, "partial_project_score": 0, "uplift_project_score": 0, }], + "energy_cost_savings": -100, "has_battery": False, + "partial_project_funding": 0, "partial_project_score": 0, "uplift_project_score": 0, + "measure_type": "roof_insulation"}], ] measures = optimiser_functions.prepare_input_measures(recs, goal="Energy Savings", needs_ventilation=False) assert measures == [] # should skip negative cost saving recs @@ -572,3 +583,262 @@ class TestCheckNeedsVentilation: ) assert result == False + + +class TestOptimiseWithScenarios: + + def test_zero_gain(self, property_instance): + input_measures = [[{'id': '0_phase=0', 'cost': 16901.01977922431, 'gain': np.float64(2.0), + 'type': 'internal_wall_insulation+mechanical_ventilation', 'innovation_uplift': 0, + 'cost_minus_uplift': 16901.01977922431, 'raw_cost': 16341.019779224309, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 0}], + [{'id': '1_phase=1', 'cost': 1197.0, 'gain': 0, 'type': 'loft_insulation', + 'innovation_uplift': 0, 'cost_minus_uplift': 1197.0, 'raw_cost': 1197.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 0}, + {'id': '2_phase=1', 'cost': 1026.0, 'gain': 0, 'type': 'loft_insulation', + 'innovation_uplift': 0, 'cost_minus_uplift': 1026.0, 'raw_cost': 1026.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 0}, + {'id': '3_phase=1', 'cost': 855.0, 'gain': 0, 'type': 'loft_insulation', + 'innovation_uplift': 0, 'cost_minus_uplift': 855.0, 'raw_cost': 855.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 0}], + [{'id': '5_phase=3', 'cost': 5343.75, 'gain': 1, 'type': 'suspended_floor_insulation', + 'innovation_uplift': 0, 'cost_minus_uplift': 5343.75, 'raw_cost': 5343.75, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 0}], + [{'id': '6_phase=4', 'cost': 1009.5600000000001, 'gain': np.float64(0.9000000000000057), + 'type': 'time_temperature_zone_control', 'innovation_uplift': 0, + 'cost_minus_uplift': 1009.5600000000001, 'raw_cost': 1009.5600000000001, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 0}, + {'id': '7_phase=4', 'cost': 18979.9, 'gain': np.float64(6.9), 'type': 'air_source_heat_pump', + 'innovation_uplift': 0, 'cost_minus_uplift': 18979.9, 'raw_cost': 18979.9, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 0}], + [{'id': '8_phase=5', 'cost': 5420.0, 'gain': np.float64(9.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 5420.0, 'raw_cost': 5420.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 3.6}, + {'id': '9_phase=5', 'cost': 6210.0, 'gain': np.float64(9.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 6210.0, 'raw_cost': 6210.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 3.6}, + {'id': '10_phase=5', 'cost': 6820.0, 'gain': np.float64(9.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 6820.0, 'raw_cost': 6820.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 3.6}, + {'id': '11_phase=5', 'cost': 7202.0, 'gain': np.float64(10.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 7202.0, 'raw_cost': 7202.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 3.915}, + {'id': '12_phase=5', 'cost': 6495.0, 'gain': np.float64(10.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 6495.0, 'raw_cost': 6495.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 3.92}, + {'id': '13_phase=5', 'cost': 7285.0, 'gain': np.float64(10.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 7285.0, 'raw_cost': 7285.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 3.92}, + {'id': '14_phase=5', 'cost': 7895.0, 'gain': np.float64(10.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 7895.0, 'raw_cost': 7895.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 3.92}, + {'id': '15_phase=5', 'cost': 5520.0, 'gain': np.float64(10.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 5520.0, 'raw_cost': 5520.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 4.0}, + {'id': '16_phase=5', 'cost': 6310.0, 'gain': np.float64(10.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 6310.0, 'raw_cost': 6310.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 4.0}, + {'id': '17_phase=5', 'cost': 6920.0, 'gain': np.float64(10.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 6920.0, 'raw_cost': 6920.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 4.0}, + {'id': '18_phase=5', 'cost': 5840.0, 'gain': np.float64(13.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 5840.0, 'raw_cost': 5840.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 5.2}, + {'id': '19_phase=5', 'cost': 6630.0, 'gain': np.float64(13.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 6630.0, 'raw_cost': 6630.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 5.2}, + {'id': '20_phase=5', 'cost': 7240.0, 'gain': np.float64(13.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 7240.0, 'raw_cost': 7240.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 5.2}, + {'id': '21_phase=5', 'cost': 8630.0, 'gain': np.float64(14.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 8630.0, 'raw_cost': 8630.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 5.655}, + {'id': '22_phase=5', 'cost': 7660.0, 'gain': np.float64(14.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 7660.0, 'raw_cost': 7660.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 5.66}, + {'id': '23_phase=5', 'cost': 8470.0, 'gain': np.float64(14.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 8470.0, 'raw_cost': 8470.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 5.66}, + {'id': '24_phase=5', 'cost': 9090.0, 'gain': np.float64(14.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 9090.0, 'raw_cost': 9090.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 5.66}, + {'id': '25_phase=5', 'cost': 7240.0, 'gain': np.float64(12.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 7240.0, 'raw_cost': 7240.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 4.79}, + {'id': '26_phase=5', 'cost': 8050.0, 'gain': np.float64(12.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 8050.0, 'raw_cost': 8050.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 4.79}, + {'id': '27_phase=5', 'cost': 8660.0, 'gain': np.float64(12.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 8660.0, 'raw_cost': 8660.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 4.79}, + {'id': '28_phase=5', 'cost': 5740.0, 'gain': np.float64(12.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 5740.0, 'raw_cost': 5740.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 4.8}, + {'id': '29_phase=5', 'cost': 6530.0, 'gain': np.float64(12.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 6530.0, 'raw_cost': 6530.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 4.8}, + {'id': '30_phase=5', 'cost': 7140.0, 'gain': np.float64(12.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 7140.0, 'raw_cost': 7140.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 4.8}, + {'id': '31_phase=5', 'cost': 8360.0, 'gain': np.float64(13.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 8360.0, 'raw_cost': 8360.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 5.22}, + {'id': '32_phase=5', 'cost': 7470.0, 'gain': np.float64(13.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 7470.0, 'raw_cost': 7470.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 5.22}, + {'id': '33_phase=5', 'cost': 8280.0, 'gain': np.float64(13.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 8280.0, 'raw_cost': 8280.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 5.22}, + {'id': '34_phase=5', 'cost': 8890.0, 'gain': np.float64(13.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 8890.0, 'raw_cost': 8890.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 5.22}, + {'id': '35_phase=5', 'cost': 5892.21, 'gain': np.float64(13.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 5892.21, 'raw_cost': 5892.21, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 5.34}, + {'id': '36_phase=5', 'cost': 5320.0, 'gain': np.float64(8.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 5320.0, 'raw_cost': 5320.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 3.2}, + {'id': '37_phase=5', 'cost': 6110.0, 'gain': np.float64(8.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 6110.0, 'raw_cost': 6110.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 3.2}, + {'id': '38_phase=5', 'cost': 6720.0, 'gain': np.float64(8.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 6720.0, 'raw_cost': 6720.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 3.2}, + {'id': '39_phase=5', 'cost': 6932.0, 'gain': np.float64(9.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 6932.0, 'raw_cost': 6932.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 3.48}, + {'id': '40_phase=5', 'cost': 6295.0, 'gain': np.float64(9.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 6295.0, 'raw_cost': 6295.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 3.48}, + {'id': '41_phase=5', 'cost': 7085.0, 'gain': np.float64(9.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 7085.0, 'raw_cost': 7085.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 3.48}, + {'id': '42_phase=5', 'cost': 7695.0, 'gain': np.float64(9.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 7695.0, 'raw_cost': 7695.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 3.48}, + {'id': '43_phase=5', 'cost': 5640.0, 'gain': np.float64(11.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 5640.0, 'raw_cost': 5640.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 4.4}, + {'id': '44_phase=5', 'cost': 6430.0, 'gain': np.float64(11.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 6430.0, 'raw_cost': 6430.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 4.4}, + {'id': '45_phase=5', 'cost': 7040.0, 'gain': np.float64(11.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 7040.0, 'raw_cost': 7040.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 4.4}, + {'id': '46_phase=5', 'cost': 8090.0, 'gain': np.float64(12.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 8090.0, 'raw_cost': 8090.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 4.785}, + {'id': '47_phase=5', 'cost': 7240.0, 'gain': np.float64(12.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 7240.0, 'raw_cost': 7240.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 4.79}, + {'id': '48_phase=5', 'cost': 8050.0, 'gain': np.float64(12.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 8050.0, 'raw_cost': 8050.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 4.79}, + {'id': '49_phase=5', 'cost': 8660.0, 'gain': np.float64(12.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 8660.0, 'raw_cost': 8660.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 4.79}, + {'id': '50_phase=5', 'cost': 5520.0, 'gain': np.float64(10.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 5520.0, 'raw_cost': 5520.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 4.0}, + {'id': '51_phase=5', 'cost': 6310.0, 'gain': np.float64(10.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 6310.0, 'raw_cost': 6310.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 4.0}, + {'id': '52_phase=5', 'cost': 6920.0, 'gain': np.float64(10.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 6920.0, 'raw_cost': 6920.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 4.0}, + {'id': '53_phase=5', 'cost': 7820.0, 'gain': np.float64(11.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 7820.0, 'raw_cost': 7820.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 4.35}, + {'id': '54_phase=5', 'cost': 6675.0, 'gain': np.float64(11.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 6675.0, 'raw_cost': 6675.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 4.35}, + {'id': '55_phase=5', 'cost': 7485.0, 'gain': np.float64(11.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 7485.0, 'raw_cost': 7485.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 4.35}, + {'id': '56_phase=5', 'cost': 8095.0, 'gain': np.float64(11.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 8095.0, 'raw_cost': 8095.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 4.35}, + {'id': '57_phase=5', 'cost': 5640.0, 'gain': np.float64(11.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 5640.0, 'raw_cost': 5640.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 4.4}, + {'id': '58_phase=5', 'cost': 6430.0, 'gain': np.float64(11.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 6430.0, 'raw_cost': 6430.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 4.4}, + {'id': '59_phase=5', 'cost': 7040.0, 'gain': np.float64(11.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 7040.0, 'raw_cost': 7040.0, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': True, 'array_size': 4.4}, + {'id': '60_phase=5', 'cost': 5692.21, 'gain': np.float64(11.0), 'type': 'solar_pv', + 'innovation_uplift': 0, 'cost_minus_uplift': 5692.21, 'raw_cost': 5692.21, + 'partial_project_funding': 0, 'partial_project_score': 0, 'uplift_project_score': 0, + 'already_installed': False, 'has_battery': False, 'array_size': 4.45}]] + + solutions = optimise_with_scenarios( + p=property_instance, + input_measures=input_measures, + budget=None, + target_gain=0, + enforce_heat_pump_insulation=True, + enforce_fabric_first=False, + already_installed_sap=0, # To be passed to output + ) + + assert solutions.empty diff --git a/recommendations/tests/test_recommendations.py b/recommendations/tests/test_recommendations.py index 747b0b2e..2218cd16 100644 --- a/recommendations/tests/test_recommendations.py +++ b/recommendations/tests/test_recommendations.py @@ -401,7 +401,7 @@ def test_adjust_ventilation_sap(sap_impact, limit, expected): ) == expected -def test_get_previous_phase_values_starting_phase(property_instance): +def test_get_previous_phase_values_phase_0_starting_phase_0(property_instance): result = Recommendations._get_previous_phase_values( rec_phase=0, starting_phase=0, @@ -411,6 +411,7 @@ def test_get_previous_phase_values_starting_phase(property_instance): assert result == { "sap": 65.0, + "sap_prediction": 65.0, "carbon": 2.4, "heat_demand": 284.0, } @@ -441,8 +442,8 @@ def test_get_previous_phase_values_single_rep(property_instance): def test_get_previous_phase_values_median(property_instance): impact_summary = [ - {"phase": 1, "representative": True, "sap": 70, "carbon": 2.0, "heat_demand": 250}, - {"phase": 1, "representative": True, "sap": 74, "carbon": 1.6, "heat_demand": 230}, + {"phase": 1, "representative": True, "sap": 70, "carbon": 2.0, "heat_demand": 250, "sap_prediction": 70}, + {"phase": 1, "representative": True, "sap": 74, "carbon": 1.6, "heat_demand": 230, "sap_prediction": 74}, ] result = Recommendations._get_previous_phase_values( From d85c44f03925ed4431278e18e61b382379d3e36c Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Tue, 24 Feb 2026 20:11:48 +0000 Subject: [PATCH 45/46] fixing incorrect condition in best practice measures --- .idea/Model.iml | 2 +- .idea/misc.xml | 2 +- recommendations/optimiser/optimiser_functions.py | 11 +++++++---- recommendations/tests/test_optimiser_functions.py | 4 ++-- 4 files changed, 11 insertions(+), 8 deletions(-) diff --git a/.idea/Model.iml b/.idea/Model.iml index e1ca1b70..c6561970 100644 --- a/.idea/Model.iml +++ b/.idea/Model.iml @@ -7,7 +7,7 @@ - + \ No newline at end of file diff --git a/.idea/misc.xml b/.idea/misc.xml index b1ee5ffa..50cad4ca 100644 --- a/.idea/misc.xml +++ b/.idea/misc.xml @@ -3,7 +3,7 @@ - + diff --git a/recommendations/optimiser/optimiser_functions.py b/recommendations/optimiser/optimiser_functions.py index a5cbf90d..6fd70c20 100644 --- a/recommendations/optimiser/optimiser_functions.py +++ b/recommendations/optimiser/optimiser_functions.py @@ -306,7 +306,6 @@ def add_best_practice_measures( solution: List[Dict[str, Any]], recommendations: Dict[int, List[List[Dict[str, Any]]]], selected: Set[str], - needs_ventilation: bool ): """ Ensures best-practice measures like ventilation and trickle vents are included @@ -327,8 +326,6 @@ def add_best_practice_measures( All recommendations for all properties, keyed by property id. selected : set Set of already selected recommendation IDs. - needs_ventilation : bool - Whether the property requires mechanical ventilation to accompany certain measures. Returns ------- @@ -338,7 +335,13 @@ def add_best_practice_measures( # Check if any selected measure requires ventilation ventilation_selected = [r for r in solution if "+mechanical_ventilation" in r["type"]] - if needs_ventilation: + # If ventilation has been selected, or one of the measures needs ventilation, we need to ensure ventilation is + # included + measures_selected_needing_ventilation = any( + x in [r["type"] for r in solution] for x in assumptions.measures_needing_ventilation + ) + + if measures_selected_needing_ventilation or len(ventilation_selected) > 0: ventilation_rec = next( (r[0] for r in recommendations[property_id] if r[0]["type"] == "mechanical_ventilation"), None diff --git a/recommendations/tests/test_optimiser_functions.py b/recommendations/tests/test_optimiser_functions.py index 08541c21..0a31ae2c 100644 --- a/recommendations/tests/test_optimiser_functions.py +++ b/recommendations/tests/test_optimiser_functions.py @@ -155,7 +155,7 @@ class TestAddBestPracticeMeasures: } selected = set() updated = optimiser_functions.add_best_practice_measures( - property_id, solution, recommendations, selected, True + property_id, solution, recommendations, selected ) assert "vent1" in updated assert "trickle1" in updated @@ -286,7 +286,7 @@ class TestIncreasingEpcE2e: total_optimised_gain = sum(m["gain"] for m in solution) assert total_optimised_gain == 17.6, "Total gain of optimised measures should meet or exceed target gain" - selected = optimiser_functions.add_best_practice_measures(p.id, solution, recommendations, selected, False) + selected = optimiser_functions.add_best_practice_measures(p.id, solution, recommendations, selected) # Flatten recommendations for output flattened = optimiser_functions.flatten_recommendations_with_defaults(p.id, recommendations, selected) From 54b00a1671d5fec57ffdd692513d7d8609f45fe8 Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Tue, 24 Feb 2026 20:13:55 +0000 Subject: [PATCH 46/46] removed incorrect ventilation input --- backend/engine/engine.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/backend/engine/engine.py b/backend/engine/engine.py index 101f6ada..8f6eca3f 100644 --- a/backend/engine/engine.py +++ b/backend/engine/engine.py @@ -1182,9 +1182,7 @@ async def model_engine(body: PlanTriggerRequest): ) # Add best practice measures (ventilation/trickle vents) - pass needs_ventilation flag - selected = optimiser_functions.add_best_practice_measures( - p.id, solution, recommendations, selected, needs_ventilation - ) + selected = optimiser_functions.add_best_practice_measures(p.id, solution, recommendations, selected) # Final flattening - we pass what the battery SAP score would be, regardless if the battery was selected recommendations[p.id] = optimiser_functions.flatten_recommendations_with_defaults( p.id, recommendations, selected, battery_sap_score