Fix computation of fraction time used by facility ID and officer type for clinic specific rescaling factors#1773
Fix computation of fraction time used by facility ID and officer type for clinic specific rescaling factors#1773sangeetabhatia03 wants to merge 25 commits intomasterfrom
Conversation
… since we removal squeeze factors
…sed_on_squeeze_factors and when checking if mode 1 appointments can go ahead
…queeze' into sb/fix-1770
marghe-molaro
left a comment
There was a problem hiding this comment.
Hi @sangeetabhatia03, I think this is looking very good, I've just added a few comments to tighten the test a bit. I have also noticed that the HealthSystem seems to be behind the version in master, so it would be good to rebase just so that we're 100% sure we're not missing anything!
| s = sim.population.props[col] | ||
| ## Not specifying the dtype explicitly here made the col a string rather than a category | ||
| ## and that caused problems later on. | ||
| sim.population.props[col] = pd.Series(s.cat.categories[0], index=s.index, dtype=s.dtype) |
There was a problem hiding this comment.
The district of residence should be 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
| # Now adjust capabilities available. | ||
| # GenericClinic has exactly the capability than needed to run all the appointments; | ||
| # Clinic1 has less capability than needed to run all the appointments; | ||
| # This will ensure rescaling factor for GenericClinic < 1 |
There was a problem hiding this comment.
The rescaling factor can never be less than 1, we don't reduce capabilities if they are underused
| nevents_clinic1 = 90 | ||
| sim = schedule_hsi_events(nevents_generic_clinic, nevents_clinic1, sim) | ||
|
|
||
| ## This hsi is only created to get the expected items; therefore the treatment_id is not important |
There was a problem hiding this comment.
if possible specify what these 'items' are, just so the intentions here are clearer
| sim.make_initial_population(n=tot_population) | ||
|
|
||
| sim.modules["HealthSystem"]._clinic_configuration = pd.DataFrame( | ||
| [{"Facility_ID": 20.0, "Officer_Type_Code": "DCSA", "Clinic1": 0.6, "GenericClinic": 0.4}] |
There was a problem hiding this comment.
Since this 0.6/0.4 split is arbitrary/never used, I would move the initialisation of the capabilities and clinics split here for facility 20 here (based on your intended plan of number of appts that you expect to be delivered in each).
|
|
||
| return sim | ||
|
|
||
| def schedule_hsi_events(notherclinic, nclinic1, sim): |
There was a problem hiding this comment.
I would try to be consistent with the use of 'other clinic' and 'generic clinic' if possible, for clarity
| @@ -2082,8 +1989,9 @@ def log_clinic_current_capabilities_and_usage(self, clinic_name): | |||
| This will log the percentage of the current capabilities that is used at each Facility Type, according the | |||
There was a problem hiding this comment.
There is no need for log_clinic_current_capabilities_and_usage to be its own fnct as log_current_capabilities_and_usage is just calling that in a loop, only keep the latter to cut down on fncs a bit
| _appt_footprint_before_running = event.EXPECTED_APPT_FOOTPRINT | ||
|
|
||
| # Mode 1: All HSI Events run with squeeze provided latter is not inf | ||
| # Mode 0: All HSI Event run, with no squeeze |
There was a problem hiding this comment.
Mode 0 should no longer be in master (removed by PR #1690), if you haven't rebased to master yet it would be good to do so, just so that we can make sure all changes are up to date
There was a problem hiding this comment.
So I've just tried to do this and it appears that even after merging master the mode 0 functionality remains.
I think the issue is with the branching history of the sb/fix-1770 branch. @tamuri could you advise on how it'd best to solve this?
There was a problem hiding this comment.
Seems like the branch for this PR did not originally branch off from master. One of the commits brought a lot of mode 0 back into this branch (by hand).
Unfortunately, no easy way to fix this. I would save a copy of the healthsystem.py in this branch. Then merge master and accept, entirely, the healthsystem.py in master. Then put back in the required changes (without bringing back in mode 0) from your saved copy.
| version https://git-lfs.github.com/spec/v1 | ||
| oid sha256:cd312903ff50d5233d81075b1f38e7879b8933e3ad7067d52c696e4f37e51eac | ||
| size 44 | ||
| Facility_ID,Officer_Type_Code,Clinic1,GenericClinic |
There was a problem hiding this comment.
These two Default.csv resource files have not been committed using Git LFS. It might be that Git LFS is not setup correctly on your machine. You will need to: (1) git rm these two files (2) make sure Git LFS is setup properly on your machine (3) add the files back and then (4) commit.
|
Closing in favour of #1781 |
* Remove squeeze factor from HS * Ensure default logging for multiple treatment ids * Replaced rescaling of capabilities based on squeeze so that it is based on load * Ensure running footprint and load is clinic specific * Add clinics running footprint and test * For test to pass for now only log Capacity_By_FacID_and_Officer for GenericClinic * Style fixes * Fix test of squeeze logging * More logging fixes in test * Update comment in test because rescaling factor cannot be less than 1 * Consistent variable names * Remove redundant function 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. * Retrieve facility id and district from appointment type needs * Additional comments in tests to explain rationale I have updated comments in the test to explain the rationale for retaining an arbitrary clinic capabilities split in test set up. * WIP: looping over clinics to log * Debugging * All but one tests pass * WIP; debugging * Fix test rescaling capabilities based on load factors * Style fixes * Style fixes Signed-off-by: sangeetabhatia03 <sangeetabhatia03@gmail.com> * Remove print statements --------- Signed-off-by: sangeetabhatia03 <sangeetabhatia03@gmail.com> Co-authored-by: sangeetabhatia03 <sangeetabhatia03@gmail.com> Co-authored-by: Tim Hallett <39991060+tbhallett@users.noreply.github.com>
This PR fixes issue #1770.
In the implementation of clinics, we did not pass clinic information to the function
frac_time_used_by_facID_and_officer, and therefore the computation therein was not clinic-specific, leading to erroneous calculations.We have now fixed this and updated other necessary objects (namely
running_total_footprint) to be clinic-specific.This PR also brings in changes from PR #1662 and PR #1689 because in the absence of these, we would have to make the whole squeeze factor computation clinic-specific, when it is intended to be removed shortly.