refactoring the recommendation impact code, with new tests

recommendations/Recommendations.py

@@ -1,7 +1,7 @@
 import pandas as pd
 import numpy as np
 from backend.Property import Property
-from typing import List
+from typing import List, Mapping
 from itertools import groupby
 from recommendations.FloorRecommendations import FloorRecommendations
 from recommendations.WallRecommendations import WallRecommendations
@@ -31,6 +31,18 @@ class Recommendations:
     High level recommendations class, which sits above the measure specific recommendation classes
     """
 
+    # Used in calculation of recommendation impact - increasing variables are features where
+    # a higher value indicates an improvement. Decreasing is the opposite
+    INCREASING_VARIABLES = ["sap"]
+    DECREASING_VARIABLES = ["carbon", "heat_demand"]
+
+    # If the recommendation is mechanical ventilation, we don't apply the rule that the new value should be higher
+    MV_INCREASING_VARIABLES = ["carbon", "heat_demand"]
+    MV_DECREASING_VARIABLES = ["sap"]
+
+    # List of models we expect predictions for, when calculation recommendation impact
+    PREDICTION_PREFIXES = ["sap_change", "heat_demand", "carbon_change"]
+
     def __init__(
         self,
         property_instance: Property,
@@ -514,15 +526,50 @@ class Recommendations:
             filtered_adjustments.append(adjustments[0])
         return filtered_adjustments
 
+    @classmethod
+    def _filter_predictions_for_property(
+        cls,
+        all_predictions: Mapping[str, pd.DataFrame],
+        property_id: str,
+    ) -> dict:
+        """
+        Utility function to filter predictions for a specific property
+        :param all_predictions: Dictionary of all predictions from the model apis
+        :param property_id: The property id to filter for
+        :return:
+        """
+
+        return {
+            f"{prefix}_predictions": (
+                all_predictions[f"{prefix}_predictions"]
+                .loc[
+                    all_predictions[f"{prefix}_predictions"]["property_id"] == property_id
+                    ]
+                .copy()
+            )
+            for prefix in cls.PREDICTION_PREFIXES
+        }
+
+    @classmethod
+    def get_monotonic_variables(cls, rec_type: str) -> tuple[List[str], List[str]]:
+        """
+        Utility function to get the monotonic variables for a specific recommendation type
+        :param rec_type: The recommendation type
+        :return:
+        """
+        if rec_type == "mechanical_ventilation":
+            return cls.MV_INCREASING_VARIABLES, cls.MV_DECREASING_VARIABLES
+        return cls.INCREASING_VARIABLES, cls.DECREASING_VARIABLES
+
     @classmethod
     def calculate_recommendation_impact(
         cls,
-        property_instance,
+        property_instance: Property,
-        all_predictions,
+        all_predictions: Mapping[str, any],
-        recommendations,
+        recommendations: Mapping[int, List],
-        representative_recommendations,
+        representative_recommendations: Mapping[int, List],
-        debug=False
+        debug: bool = False
-    ):
+    ) -> (Mapping[int, List], List[Mapping[str, any]]):
 
         """
         Given predictions from the model apis, with method will update the recommendations with the predicted
@@ -539,31 +586,20 @@ class Recommendations:
         :param debug: boolean, indicating if the function is running in debug mode. The only difference is that
                       adjustments are returned for testing
 
-        :return:
+        :return: Updated recommendations with predicted impact, and a list of impacts by phase
         """
+        property_predictions = cls._filter_predictions_for_property(
+            all_predictions, str(property_instance.id)
+        )
 
-        property_predictions = {
+        # shallow copy intentional - we're going to modify the internals
-            prefix + "_predictions": all_predictions[prefix + "_predictions"][
-                all_predictions[prefix + "_predictions"]["property_id"] == str(property_instance.id)
-                ].copy() for prefix in ["sap_change", "heat_demand", "carbon_change"]
-        }
-
         property_recommendations = recommendations[property_instance.id].copy()
 
         representative_recs = representative_recommendations[property_instance.id].copy()
         representative_ids = [r["recommendation_id"] for r in representative_recs]
 
-        increasing_variables = ["sap"]
-        decreasing_variables = ["carbon", "heat_demand"]
-
-        # If the recommendation is mechanical ventilation, we don't apply the rule that the new value should be higher
-        mv_increasing_variables = ["carbon", "heat_demand"]
-        mv_decreasing_variables = ["sap"]
-
         # We allow for negative phase
-        starting_phase = min(
+        starting_phase = min(rec["phase"] for recs in property_recommendations for rec in recs)
-            rec["phase"] for recs in property_recommendations for rec in recs
-        )
 
         # We keep a history of adjustments we have made, so that we ensure that we adjust future
         # phases for SAP
@@ -602,7 +638,7 @@ class Recommendations:
                     prefix: property_predictions[prefix + "_predictions"][
                         property_predictions[prefix + "_predictions"]["recommendation_id"] == str(
                             rec["recommendation_id"]
-                        )]["predictions"].values[0] for prefix in ["sap_change", "heat_demand", "carbon_change"]
+                        )]["predictions"].values[0] for prefix in cls.PREDICTION_PREFIXES
                 }
 
                 # We structure this so that depending on the phase, we capture the previous phase impacts and
@@ -669,12 +705,7 @@ class Recommendations:
                 # However, if the recommendation is mechanical ventilation, this can have a negative SAP impact so
                 # we don't apply this rule
 
-                if rec["type"] == "mechanical_ventilation":
+                phase_increasing_variables, phase_decreasing_variables = cls.get_monotonic_variables(rec["type"])
-                    phase_increasing_variables = mv_increasing_variables
-                    phase_decreasing_variables = mv_decreasing_variables
-                else:
-                    phase_increasing_variables = increasing_variables
-                    phase_decreasing_variables = decreasing_variables
 
                 for v in phase_increasing_variables:
                     current_phase_values[v] = (
@@ -718,7 +749,21 @@ class Recommendations:
                         property_instance.lighting["low_energy_proportion"]
                     )
 
-                    property_phase_impact["sap"] = min(property_phase_impact["sap"], lighting_sap_limit)
+                    # add an adjustment
+                    proposed_sap_impact = min(property_phase_impact["sap"], lighting_sap_limit)
+                    if proposed_sap_impact != property_phase_impact["sap"]:
+                        # Store the sap adjustment. The proposed sap impact will always be less
+                        # than the current sap impact, so the adjustment is always positive
+                        # as we subtract it from the future phases
+                        adjustments.append(
+                            {
+                                "recommendation_id": rec["recommendation_id"],
+                                "phase": rec["phase"],
+                                "sap_adjustment": property_phase_impact["sap"] - proposed_sap_impact,
+                            }
+                        )
+
+                    property_phase_impact["sap"] = proposed_sap_impact
                     property_phase_impact["carbon"] = min(
                         property_phase_impact["carbon"], rec["co2_equivalent_savings"]
                     )
@@ -812,8 +857,9 @@ class Recommendations:
                             {
                                 "recommendation_id": rec["recommendation_id"],
                                 "phase": rec["phase"],
-                                # If we've made an adjustment, it will be positive
+                                # If we've made an adjustment, we will be increasing the number of SAP
-                                "sap_adjustment": proposed_impact - property_phase_impact["sap"],
+                                # points. Since, we subtract adjustments, this number should be negative
+                                "sap_adjustment": property_phase_impact["sap"] - proposed_impact,
                             }
                         )
                     property_phase_impact["sap"] = proposed_impact

tox.ini

@@ -7,6 +7,7 @@ passenv = EPC_AUTH_TOKEN
 description = Install dependencies and run tests
 deps =
     -rbackend/engine/requirements.txt
+    -rbackend/app/requirements/requirements.txt
     -rtest.requirements.txt
 commands = pytest