-
Notifications
You must be signed in to change notification settings - Fork 16
Fix branching issues in PR #1662, #1689, and #1773 #1781
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Fix branching issues in PR #1662, #1689, and #1773 #1781
Conversation
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. |
|
The rest of the changes made so far seem good to me and well commented and clear. |
|
Pros of keeping it in:
Cons of keeping it in:
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! |
|
Do we need the two resource files that were added in #1773? @sangeetabhatia03 - will wait for your review. |
log_current_capabilities_and_usage called log_clinic_current_capabilities_and_usage looping over the clinics. I have now removed log_clinic_current_capabilities_and_usage, and moved the looping logic to where the capabilities are logged.
I have updated comments in the test to explain the rationale for retaining an arbitrary clinic capabilities split in test set up.
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 keykey="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).