Fix branching issues in PR #1662, #1689, and #1773

[marghe-molaro]

This PR manually manually reproduces changes that had originally been implemented in PR #1662, #1689, and #1773 from the current version of master, to ensure that all updates to the HealthSystem that have occurred since are accounted for.

PRs #1662 and #1689: removed the concept of squeeze factor from the HealthSystem entirely.
The only difference in this implementation compared to that of PR #1662 is that although "squeeze_factor" is still logged (always as zero), to ensure consistency in logging, this is no longer passed to the logging fnc as an argument. Reasoning for this is to emphasise that currently the logged value is not dynamically estimated, but @tamuri let me know if you disagree.

PR #1773: had introduced fixes to the estimation of the load factor to ensure that this is clinic-compatible, these have now been copied over. Changes introduced here are that the clinic should should not be an optional input to frac_time_used_by_facID_and_officer: ensuring this appears to have interfered with the logging of key key="Capacity_By_FacID_and_Officer", so it would be great @sangeetabhatia03 if you could address this.

As discussed over slack, reviews provided to PR #1773 should be addressed in this PR instead.

@sangeetabhatia03: an additional test that would be great to have is one where the rescaling across clinics happens 'organically' when undergoing a mode transition; this would ensure not only that the rescaling based on footprints works as expected, but that this is also consistent with mode change and with a previous period in mode 1. This is ultimately the functionality that we will need for all TLO runs, so having a test that clears that this is all working as it should would be very useful. For the purposes of the test, the sim can be very small (everyone in one district, running only for one year i.e. mode switch in 2011).

[tbhallett]

PRs #1662 and #1689: removed the concept of squeeze factor from the HealthSystem entirely.
The only difference in this implementation compared to that of PR #1662 is that although "squeeze_factor" is still logged (always as zero), to ensure consistency in logging, this is no longer passed to the logging fnc as an argument. Reasoning for this is to emphasise that currently the logged value is not dynamically estimated, but @tamuri let me know if you disagree

I can see from this, that it's an intentional decision: but I am wondering about whether we should pull out all references to 'squeeze_factor' (including logs and the HSI_Event definition)? Of course, I can see that this is somewhat unnecessary as the changes for now work fine (and setting it to zero by default is ok). But, I'm also thinking that leaving the vestiges of 'squeeze_factor' could invite confusion in the future. Happy to be guided by you all on this though.

[tbhallett]

The rest of the changes made so far seem good to me and well commented and clear.

[marghe-molaro]

Pros of keeping it in:

• If at any point we do want to reintroduce the concept of squeeze it will be very straightforward to do so;
• We avoid having to remove every mention of squeeze factor from the code (there are a lot!) which is trivial but just a bit of a faff;

Cons of keeping it in:

• Might create confusion for users and generally make things a bit untidy

Given that (hopefully soon!) we plan to restructure the healthsystem, it might be something that can be decided then?

[tbhallett]

Pros of keeping it in:

• If at any point we do want to reintroduce the concept of squeeze it will be very straightforward to do so;
• We avoid having to remove every mention of squeeze factor from the code (there are a lot!) which is trivial but just a bit of a faff;

Cons of keeping it in:

• Might create confusion for users and generally make things a bit untidy

Given that (hopefully soon!) we plan to restructure the healthsystem, it might be something that can be decided then?

Fair enough.... and it's true that it's one of many vestiges that we would hope to sweep away in a future overhaul.

Ok, so let's finish this one, which does the heavy logical lifting. And following this, I could do a 'Find and Replace... ' style one, to expunge any mention of it, and we can debate it then!

[tamuri]

Do we need the two resource files that were added in #1773? @sangeetabhatia03 - will wait for your review.

[marghe-molaro]

Hi @sangeetabhatia03,
Thank you for identifying and fixing the issues in logging the Capacity_By_FacID_and_Officer and in the <test_rescaling_capabilities_based_on_load_factors>.

I have added a small extra test in <test_rescaling_capabilities_based_on_load_factors> following your suggestion. Also, I think you are right that the exact threshold of the ratio is somewhat arbitrary as it is convolved with demand, capabilities, and noise; I have now modified this so that it is somewhat more sound in terms of the initial rescaling factor, and that it passes. (This is additionally GenericClinic-specific now). So I think this test is now ok!

[sangeetabhatia03]

Hi @marghe-molaro / @tbhallett and @tamuri this PR is now ready for your review. Comments on the changes you suggested in the original PR:

• I have modified the test so that the district of residence is derived from the location associated with the selected Facility_ID, so that we can be sure the district to which the population is assigned matches the district of the facility. I have also attempted to reduce hardcoding dictionary keys.

• I have updated comments on the test so that they better reflect our intentions e.g. that rescaling factor >= 1; indicating the "items" needed by HSI event. I have also modifed variable names to be consistent - i.e. ngenericclinic instead of notherclinic.

• In the interest of expediency, I have not at the moment modified the test to derive the capabilities split dynamically based on the number of events we expect to schedule. I'l shortly create another PR where I'll improve this test as well as the test_mode2_clinics.

• Removed log_clinic_current_capabilities_and_usage and added clinic as a parameter to log_current_capabilities_and_usage

• Added clinic Resource files using git lfs

@tamuri I'll be grateful if you can particularly look at my comments in the function frac_time_used_by_facID_and_officer where the following if condition did not make sense to me as it will always evaluate to False.

if (_facID_and_officer == facID_and_officer or _facID_and_officer is None)

[tamuri]

@tamuri I'll be grateful if you can particularly look at my comments in the function frac_time_used_by_facID_and_officer where the following if condition did not make sense to me as it will always evaluate to False.

This looks like one for Tim or Margherita to confirm.

[marghe-molaro]

@tamuri I'll be grateful if you can particularly look at my comments in the function frac_time_used_by_facID_and_officer where the following if condition did not make sense to me as it will always evaluate to False.

This looks like one for Tim or Margherita to confirm.

@sangeetabhatia03 I think we are happy that everything works as expected with these modifications in, so happy to go ahead with it

[tamuri]

Looks fine - need to remove some print statements.

[tamuri]

These lines need to be removed (or turned into normal logger entries)

[tamuri]

Remove print statement