Fixes for diarrhoea generic first appointment logic and updating of individual properties across modules

[matt-graham]

Related to #1348 and #1366.

As discussed in software engineering meeting today, this PR addresses two known issues with current refactored generic first appointments logic:

1. A change in the conditions on which the diarrhoea module does or does not perform an action on a generic first appointment introduced accidentally by the refactoring in Avoid DataFrame accesses and standardise HSI scheduling interactions during first appointments #1292 is fixed to return to the previous behaviour (not performing any further action if the individual is over the age of 5 or does not have any diarrhoea symptoms).
2. The issue identified by @marghe-molaro in Batch of runs submitted from PR #1308 before/after review&master merge inconsistent  #1348 (comment), that is that in
   

   TLOmodel/src/tlo/methods/hsi_generic_first_appts.py

   Lines 80 to 103 in df25c76

   	for module in modules.values():
   	module_patient_updates = getattr(module, self.MODULE_METHOD_ON_APPLY)(
   	patient_id=self.target,
   	patient_details=patient_details,
   	symptoms=symptoms,
   	diagnosis_function=self._diagnosis_function,
   	consumables_checker=self.get_consumables,
   	facility_level=self.ACCEPTED_FACILITY_LEVEL,
   	treatment_id=self.TREATMENT_ID,
   	random_state=self.module.rng,
   	)
   	# Record any requested DataFrame updates, but do not implement yet
   	# NOTE: |= syntax is only available in Python >=3.9
   	if module_patient_updates:
   	proposed_patient_details_updates = {
   	**proposed_patient_details_updates,
   	**module_patient_updates,
   	}
   	# Perform any DataFrame updates that were requested, all in one go.
   	if proposed_patient_details_updates:
   	df.loc[
   	self.target, proposed_patient_details_updates.keys()
   	] = proposed_patient_details_updates.values()

   
   updates to an individual properties by previous modules in the loop are not reflected necessarily in the patient_details structure passed to the do_at_generic_first_appt method of the current module in the loop as the updates are only made to the dataframe after the loop has completed. Due the subsequent changes to the PatientDetails type made in Access individual properties lazily with memoization in PatientDetails type #1315, this will actually only be an issue if an update is made to a property by a module that has previously been read (and cached) by that or another module and then a subsequent module reads the same property again (as it will get the initial cached value). I'm not sure there are cases where that chain actually happens currently given only very few modules actually output updates to the individual properties and they are for properties usually specific to the module, so this possibly explains why @willGraham01 was able to get consistent dataframe checksums when working on Inconsistency fix between HEAD of master and older PR branches. #1366 even without this fix.

The fix for (2) here uses the simple approach I mentioned in #1348 (comment) which will loose some of the performance benefits we currently have by doing updates to the dataframe whenever a module returns properties to update (rather than doing all updates at once). In the longer term #1387 should fix the same issue in a more performant manner.

[matt-graham]

@marghe-molaro this PR should hopefully deal with the issues so once this is reviewed / merged to master you should be able to merge in to your branch for analysis runs and re-run to check if we restore original behaviour. If it does then we can close #1348.

[marghe-molaro]

Thank you very much @matt-graham!
Now that @tamuri has approved these changed I will merge these fixes into commit a91d60c, which produced the plots in which we first spotted an issue. This will allow us to establish whether the issues fixed in this PR were behind the discrepancy.

[marghe-molaro]

@matt-graham it appears there's an issue with the diarrhoea test

[matt-graham]

It looks like Will also updated the tests that are failing here in #1366 to force individual to have diarrhoea symptoms - currently investigating why that was needed and will update test here if necessary.

[matt-graham]

It looks like Will also updated the tests that are failing here in #1366 to force individual to have diarrhoea symptoms - currently investigating why that was needed and will update test here if necessary.

I realized the manual setting of diarrhoea symptoms was now necessary as prior to the refactoring we called a method Diarrhoea.do_when_presentation_with_diarrhoea which assumed symptoms had already been checked, while post refactoring the logic of this method has been moved to the Diarrhoea.do_at_generic_first_appt method which now, with the fix in this PR, exits straight away if there isn't "diarrhoea" present in the passed set of symptoms. Have now updated the test to manually create a set of symptoms with "diarrhoea" present where applicable so this should hopefully fix the failure.

[matt-graham]

As this is just a change to the test to keep it up to sync with the changes in the module, this shouldn't affect the validity of any runs with the changes here merged in before the last commit @marghe-molaro, just in case you've already set anything off!

[marghe-molaro]

Hi @matt-graham and @tamuri, quick update: @tbhallett noticed that this PR was based on a version of master that had brought in changes to the HIV/TB/malaria modules which lead to a rescaling of DALYs and scheduled HSIs for those diseases (#1273). By comparing the original runs which included the error with the fixes brought in this PR (#1389), it has therefore become difficult to isolate the effect of modules reordering on the changes to the HIV DALYs evolution.

I will submit a new set of test runs from the current version of master where the only change added is the scrambling of the disease modules to recover the order in the original code vs the one introduced by PR #1292. Hopefully this will allow us to establish once and for all whether reordering the modules during the first generic appts leads to any statistically significant changes. Let me know if you have any thoughts on this.

[matt-graham]

I will submit a new set of test runs from the current version of master where the only change added is the scrambling of the disease modules to recover the order in the original code vs the one introduced by PR #1292. Hopefully this will allow us to establish once and for all whether reordering the modules during the first generic appts leads to any statistically significant changes. Let me know if you have any thoughts on this.

Hi @marghe-molaro that seems sensible to me.