Repository: UCL/TLOmodel
Issue category: Performance Issue

Issue:
Population dataframe accesses in `HSI_Contraception_FamilyPlanningAppt.EXPECTED_APPT_FOOTPRINT` are performance bottleneck
Look at the most recent successful profiling run results

https://github-pages.ucl.ac.uk/TLOmodel-profiling/_static/profiling_html/schedule_1226_dbf33d69edadbacdc2dad1fc32c93eb771b03f9b.html

calls to the `HSI_Contraception_FamilyPlanningAppt.EXPECTED_APPT_FOOTPRINT` property are consuming a lot of run time, with a total of 2570s spent in evaluating this property out of a total run time of 27&thinsp;400s (9.3%). There are three functions calling `HSI_Contraception_FamilyPlanningAppt.EXPECTED_APPT_FOOTPRINT` with a significant amount of time recorded in each call path

- `HealthSystem.check_hsi_event_is_valid` (1110s)
- `HSI_Contraception_FamilyPlanningAppt.initialise` (545s)
- `HSI_Contraception_FamilyPlanningAppt.run` (911s)

and in all cases the vast majority of the time in the `HSI_Contraception_FamilyPlanningAppt.EXPECTED_APPT_FOOTPRINT` call is spent in `.loc` indexing operations on the population dataframe.

Looking at the implementation of the property

https://github.com/UCL/TLOmodel/blob/5b4f328e5fc0df469eec40bcf1110a555326aa80/src/tlo/methods/contraception.py#L1150-L1170

we can see this must be arising from the access to the current contraception method property for the individual the HSI event is targetting in the line

```Python
current_method = self.sim.population.props.loc[person_id].co_contraception 
```

<details>
<summary>
This pattern of the `EXPECTED_APPT_FOOTPRINT` being a property rather than an attribute seems to also be used by a few other HSI events
</summary>

`HSI_Hiv_StartOrContinueTreatment`

https://github.com/UCL/TLOmodel/blob/5b4f328e5fc0df469eec40bcf1110a555326aa80/src/tlo/methods/hiv.py#L2968-L2986

`HSI_Tb_StartTreatment`

https://github.com/UCL/TLOmodel/blob/5b4f328e5fc0df469eec40bcf1110a555326aa80/src/tlo/methods/tb.py#L2175-L2183

`HsiBaseVaccine`

https://github.com/UCL/TLOmodel/blob/5b4f328e5fc0df469eec40bcf1110a555326aa80/src/tlo/methods/epi.py#L407-L431
</details>

I'm assuming this has been implemented like this to allow the expected appointment footprint to be based on the target's properties at the point at which it is run rather than when it is scheduled? Tagging @tbhallett, @BinglingICL and @EvaJanouskova as I think this was originally implemented in #826. 

One thing I'm not sure about is how this pattern interacts with this logic in `HSI_Event.initialise`

https://github.com/UCL/TLOmodel/blob/5b4f328e5fc0df469eec40bcf1110a555326aa80/src/tlo/methods/hsi_event.py#L333-L338

which reassigns `self.EXPECTED_APPT_FOOTPRINT` in certain cases, in which case it would end up as a plain (static) attribute rather than a property. As `HSI_Event.initialise` appears to be called in `HealthSystem.schedule_hsi_event`, that is at the time the HSI event is scheduled, this means in some cases even when `EXPECTED_APPT_FOOTPRINT` is defined as a property it will be 'locked-in' as a static attribute with values taken from population dataframe at point at which it is scheduled.

As accessing the population dataframe dynamically when `EXPECTED_APPT_FOOTPRINT` is a property seems to be a performance bottleneck, and given it does seem to be guaranteed this will actually be done at the point the HSI event is run rather than scheduled, it seems that enforcing that `EXPECTED_APPT_FOOTPRINT` is an attribute which is contructed when the HSI event is constructed (when scheduling) might be a good idea to both keep the behaviour consistent and allow us to reduce the population dataframes by just doing once on construction rather than each access of `EXPECTED_APPT_FOOTPRINT`.


Hints:
@BinglingICL, I think it is okay to set the EXPECTED_APPT_FOOTPRINT with the same logic when HSI is scheduled instead of when it is run, isn't it?
> [@BinglingICL](https://github.com/BinglingICL), I think it is okay to set the EXPECTED_APPT_FOOTPRINT with the same logic when HSI is scheduled instead of when it is run, isn't it?

Hi Eva, it seems so to me, if you meant the way to assign appt footprint within the property.

Patch:
diff --git a/src/tlo/methods/contraception.py b/src/tlo/methods/contraception.py
index ab6c633f4c..09cb394804 100644
--- a/src/tlo/methods/contraception.py
+++ b/src/tlo/methods/contraception.py
@@ -1146,12 +1146,12 @@ def __init__(self, module, person_id, new_contraceptive):
 
         self.TREATMENT_ID = "Contraception_Routine"
         self.ACCEPTED_FACILITY_LEVEL = _facility_level
-
-    @property
-    def EXPECTED_APPT_FOOTPRINT(self):
-        """Return the expected appt footprint based on contraception method and whether the HSI has been rescheduled."""
-        person_id = self.target
         current_method = self.sim.population.props.loc[person_id].co_contraception
+        self.EXPECTED_APPT_FOOTPRINT = self._get_appt_footprint(current_method)
+
+
+    def _get_appt_footprint(self, current_method: str):
+        """Return the appointment footprint based on contraception method and whether the HSI has been rescheduled."""
         if self._number_of_times_run > 0:  # if it is to re-schedule due to unavailable consumables
             return self.make_appt_footprint({})
         # if to switch to a method
@@ -1165,9 +1165,11 @@ def EXPECTED_APPT_FOOTPRINT(self):
         elif self.new_contraceptive in ['male_condom', 'other_modern', 'pill']:
             return self.make_appt_footprint({'PharmDispensing': 1})
         else:
+            warnings.warn(
+                "Assumed empty footprint for Contraception Routine appt because couldn't find actual case.",
+                stacklevel=3,
+            )
             return self.make_appt_footprint({})
-            warnings.warn(UserWarning("Assumed empty footprint for Contraception Routine appt because couldn't find"
-                                      "actual case."))
 
     def apply(self, person_id, squeeze_factor):
         """If the relevant consumable is available, do change in contraception and log it"""
@@ -1266,6 +1268,8 @@ def apply(self, person_id, squeeze_factor):
         ):
             self.reschedule()
 
+        return self._get_appt_footprint(current_method)
+
     def post_apply_hook(self):
         self._number_of_times_run += 1
 


Test patch: