-
Notifications
You must be signed in to change notification settings - Fork 9
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
Include task-shifting #1280
base: master
Are you sure you want to change the base?
Include task-shifting #1280
Conversation
@marghe-molaro we maybe do want some measure of the amount of task-shifting which occurs during a simulation so we could quantify for example, what would we gain if nurses were allowed to spend 25% of their time on pharmacy tasks? Or conversely, if global task-shifting is permitted, how much nurses time would be required to fill the gaps through pharmacists being under-resourced? We could do this through counterfactuals of course, but having direct measures is always useful as counterfactuals quickly diverge making run-by-run comparisons tricky. |
@marghe-molaro also noting here the important role that DCSAs may play when we consider task-shifting - they are already doing MDA and child health days for example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for this, @marghe-molaro
This is all very nice and seems to work well.
My suggestions boil-down to three things:
-
In the test, I think we should repeat that experiment with/without global task shifting and see the difference.
-
Interface-wise, I think let this only and fully controlled by a csv ResourceFile describing the global task shifting we want (If it's blank it means we don't want any). And let's not have the key-word argument thing, as it's not necessary.
-
Logic and logging-wise: I thought that we could perhaps store onto the HSI Event itself the 'task-shifted' appointment and then manufacture that this is returned from the HSI_Event (as the 'ACTUAL_APPT_FOOTPRINT`). That way the updating of capabilities and the logging of what actual is used all happens automatically. In addition we could have a flag that this is a "task shifted" appointment, which would also flow into the logs.
src/tlo/methods/healthsystem.py
Outdated
@@ -599,6 +603,7 @@ def __init__( | |||
beds_availability: Optional[str] = None, | |||
randomise_queue: bool = True, | |||
ignore_priority: bool = False, | |||
include_task_shifting: bool = False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why a module keyword argument and a module Parameter? I think best to stick to it being ONLY a Parameter, if possible.
src/tlo/methods/healthsystem.py
Outdated
self.global_task_shifting = { | ||
'Pharmacy': (['Nursing_and_Midwifery', 'Clinical'], [1.5,1]), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think more pythonic to write as we don;t then rely on ordering of these two lists, and the relationship between the numbers (1.5, 1.0) and strings ('Nursing', 'Clinical') is made explicit. (Also when it's used you're using zip to iterate through these things together, but if it were a dict, you could just use .items()
)
self.global_task_shifting = { | |
'Pharmacy': (['Nursing_and_Midwifery', 'Clinical'], [1.5,1]), | |
} | |
self.global_task_shifting = { | |
'Pharmacy': {'Nursing_and_Midwifery': 1.5, 'Clinical': 1.0}, | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also - shouldn't think be read in from a .csv ResourceFile, so that we can edit this in different simulations?
Structure of .csv file:
Nursing_and_Midwifery,1.5
Clinical,1.0
This would actually made the Parameter (include_task_shifting) redundant, because if we didn't want to allow any (global) task shifting rules, then this file would be blank.
src/tlo/methods/healthsystem.py
Outdated
out_of_resources = True | ||
|
||
if self.sim.modules['HealthSystem'].include_task_shifting: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if self.sim.modules['HealthSystem'].include_task_shifting: | |
if self.module.include_task_shifting: |
# Unpack values | ||
new_officer_no_facility, time_scaling = task_shift_options_for_officer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why unpack only to zip in the following line? I think we can skip this line.
out_of_resources = False | ||
|
||
# Once we've found available officer to replace, no need to go through other | ||
# options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# options | |
# options for other officers to task-shift to. |
src/tlo/methods/healthsystem.py
Outdated
# WARNING: the logged appt footprint does not contain information | ||
# on whether task-shifting was performed or not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, this would be another reason to let the HSI return actual footprint that its provided with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See reply to comment above
src/tlo/methods/healthsystem.py
Outdated
@@ -2827,6 +2922,7 @@ class HealthSystemChangeParameters(Event, PopulationScopeEventMixin): | |||
"""Event that causes certain internal parameters of the HealthSystem to be changed; specifically: | |||
* `mode_appt_constraints` | |||
* `ignore_priority` | |||
* `include_task_shifting` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above, it seems duplicative to me to provide two ways to modify this behaviour. I think it should just be through Parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes you are right, the reason this wasn't optimised at this stage is that we're still not sure whether we wanted to allow for a global and HSI-specific task-shifting approach (in which case e.g. include_task_shifting could be categorical - None, Global-level, HSI-level and we would have an additional resource file in case of global taskshifting, one for HSI-based on etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's always the case that we should modify the behaviour of the module through Parametees rather than kwargs though, as that it has the Scenario class expects to pass through changes.
tests/test_healthsystem.py
Outdated
# Check that all events could run, even if Pharmacy capabilities were set to zero, | ||
# and that when task-shifting first option (nurses) where always preferentially chosen over second | ||
# one (clinicians) | ||
assert hs_output['did_run'].sum() == Ntarget | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To strengthen the test and see the impact of task-shifting versus no task-shifting, I think it would be good to repeat this for with/without global task-shifting so we can see the difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding me to this @marghe-molaro. I'm unable to offer comments on the code, but the approach sounds right to me.
Many thanks @tdm32 and @tbhallett for your comments! Tara's points:
Tim's points:
I replied to some of your comments directly Tim. |
…his will be required for comparing scenarios
Hi @tbhallett and @tdm32, (Note: Tests are failing on github but running fine locally.) Following the discussion on Wed, I have now made the following modifications to this PR: _appt_times (which links appt_footprints to the officers× required to perform the appointment) is now inflated in pre-processing to include new appt_footprints, which account for all possible task-shifting that may take place given the policy specified by global_task_shifting. At the time of running HSIs in mode 2, if task-shifting is required a tag "withTS..." with all relevant task-shifting info will be added to the actual_appt_footprint returned by the HSI, such that times requested from each officer can be quickly looked-up from the expanded _appt_times (and hence subtracted from daily capabilities) rather than having to be recalculated for each HSI at run time. In doing so the TS can be logged as having occurred with details of how that took place directly as part of the logged appointment footprint. It is difficult to say whether this approach will be faster than the one we had implemented before, as this will very much depend on how much TS actually takes place. It shouldn't impact runs where TS is not included. This approach however allows us to easily log TS as having taken place in all relevant detail. |
@tbhallett following today's Health Econ discussion I would suggest pausing review of this PR until we have a clearer idea of how we want to enforce task-shifting, in particular if we want this to be more HSI-driven. |
Found this ResourceFile on another approach (#1296) but not here. |
Initial task-shifting implementation, currently assuming that the health-system adopts a "global policy" on task shifting. We will potentially transition to treatment-specific after health-econ discussion on 8th March.
PR includes test (test_task_shifting_in_mode_2)
Other notes: