Generate HSI_Contraception_FamilyPlanningAppt.EXPECTED_APPT_FOOTPRINT at class initialisation

[matt-graham]

Resolves #1467

Changes HSI_Contraception_FamilyPlanningAppt.EXPECTED_APPT_FOOTPRINT from being a property to a static attribute, with the value evaluated when the class (HSI event) is initialised (scheduled).

This may change the value associated with EXPECTED_APPT_FOOTPRINT as it is now depends on the targets properties (specifically the current contraception method indicated by the categorical co_contraception property) at the time of scheduling rather than evaluation of EXPECTED_APPT_FOOTPRINT. Marking this as draft for now as will need @EvaJanouskova and/or @BinglingICL to verify if this change in behaviour is reasonable.

As discussed in #1467 repeated dataframe accesses when dynamically evaluating EXPECTED_APPT_FOOTPRINT for this HSI event where becoming a significant performance bottleneck - from some short profiled runs locally the changes here give around a 20% reduction in run time.

[matt-graham]

From running the tests locally it looks like these changes break at least one test in tests/test_contraception.py:

_______________________________________________________________________________ test_record_of_appt_footprint_for_switching_to_methods[83563095832589325021] ________________________________________________________________________________
tests/test_contraception.py:485: in test_record_of_appt_footprint_for_switching_to_methods
    assert is_list_longer_than_length_of_one_and_with_first_element_nonblank_and_subsequent_blank(
E   AssertionError: assert False
E    +  where False = <function test_record_of_appt_footprint_for_switching_to_methods.<locals>.is_list_longer_than_length_of_one_and_with_first_element_nonblank_and_subsequent_blank at 0x7fb3ac5d45e0>([{'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, ...])
E    +    where [{'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, ...] = <function test_record_of_appt_footprint_for_switching_to_methods.<locals>.get_appt_footprints at 0x7fb3ac614c20>(switch_from='not_using', switch_to='female_sterilization', consumables_available=False)

I am not entirely sure what this test is meant to be checking. The full output of the call get_appt_footprints(switch_from='not_using', switch_to='female_sterilization', consumables_available=False) which is passed to is_list_longer_than_length_of_one_and_with_first_element_nonblank_and_subsequent_blank is

[{'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}, {'MinorSurg': 1}]

with the last condition (subsequent elements after first blank) failing.

[matt-graham]

My understanding of why the test is failing is that the HSI event reschedules itself if a lack of consumables meant the change to contraception method could not be enacted

TLOmodel/src/tlo/methods/contraception.py

Lines 1266 to 1269 in 4a67b48

	if (not co_administrated) and (
	self._number_of_times_run < self.module.parameters['max_number_of_runs_of_hsi_if_consumable_not_available']
	):
	self.reschedule()

with the reschedule function passing in self as the HSI event instance

TLOmodel/src/tlo/methods/contraception.py

Lines 1276 to 1282 in 4a67b48

	def reschedule(self):
	"""Schedule for this same HSI_Event to occur tomorrow."""
	self.module.sim.modules['HealthSystem'].schedule_hsi_event(hsi_event=self,
	topen=self.sim.date + pd.DateOffset(days=1),
	tclose=None,
	priority=1)

This means in any subsequent runs of HSI event apply method the value of EXPECTED_APPT_FOOTPRINT will remain what is was when event was first scheduled and __init__ method called (and so be based on the current contraception method at that point), as the rescheduled events do not create a new HSI event instance.

I believe the intended interface for communicating changes to the appointment footprint from the initial expected footprint at the point of scheduling is for the apply method to return the actual appointment footprint. Adding a line

TLOmodel/src/tlo/methods/contraception.py

Line 1271 in 4a67b48

return self._get_appt_footprint(current_method)

to the end of the apply method to return the appointment footprint based on current_method in the call to apply appears to fix the previously failing test.

I initially also added a line current_method = _new_contraceptive to the if block

TLOmodel/src/tlo/methods/contraception.py

Lines 1256 to 1262 in 4a67b48

	if current_method != _new_contraceptive:
	# Do the change:
	self.module.do_and_log_individual_contraception_change(
	woman_id=self.target,
	old=current_method,
	new=_new_contraceptive
	)

so that current_method at the end of the method when self._get_appt_footprint is called reflects the method after a change has been enacted but this led to another test failure

FAILED tests/test_contraception.py::test_record_of_appt_footprint_for_switching_to_methods[83563095832589325021] - AssertionError: assert [{'FamPlan': 1}] == [{'PharmDispensing': 1}]

so I am assuming the actual appointment footprint should be based on the current contraception method at the point HSI event is being run not after it has run (and the method has potentially changed).

[matt-graham]

/run profiling

[BinglingICL]

Hi Matt, thanks for the neat and clear change and careful checks. I think the changes you've made look fine and agree with your saying that "actual appointment footprint should be based on the current contraception method at the point HSI event is being run not after it has run (and the method has potentially changed)".

Would be grateful if Eva @EvaJanouskova could double check the logic.