-
Notifications
You must be signed in to change notification settings - Fork 15
Param Revamp - Pregnancy Supervisor #1733
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?
Conversation
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.
Great work Mariana on what is, as always, a very annoying module to make change to. Please see my comments, i think we could work on the names of some of the new parameters being more explict but happy to chat on that.
sim.schedule_event( | ||
PregnancySupervisorEvent(self), | ||
sim.date + DateOffset(days=params['initial_event_scheduling_offset_days']) | ||
) |
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.
wonder if we should just delete this as its 0 days...
# the next week | ||
if anc_month == 2: | ||
days_until_anc = self.rng.randint(0, 6) | ||
if anc_month == params['anc_care_seeking_initial_weeks']/4: |
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 can see what youve done here but maybe the parameter names could be a bit more descriptive...? the code is a little less readible without seeing the values (which obviously is not your fault!!)
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.
could it be "gestational_age_at_which_anc_is_sought" or something like that?
poss_day_onset = (27 - 22) * 7 | ||
current_gestation = df.at[person, 'ps_gestational_age_in_weeks'] | ||
|
||
if current_gestation == 22: |
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.
not following why these 'floating' values can remain?
poss_day_onset = (31 - 27) * 7 | ||
onset_day = self.rng.randint((params['preterm_labour_min_weeks'] - 22) * 7, poss_day_onset) | ||
elif current_gestation == 27: | ||
poss_day_onset = (params['preterm_labour_gestation_27_max'] - 27) * 7 |
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 like the numbers in these parameter names - could we do similar for the ones above where anc is sheduled??
new_pregnancy = df.is_alive & df.is_pregnant & (df.ps_gestational_age_in_weeks == 3) | ||
new_pregnancy = ( | ||
df.is_alive & df.is_pregnant & | ||
(df.ps_gestational_age_in_weeks == params['gestation_week_for_risk_application']) |
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.
"gestation_week_for_first_risk_application"
'anc_care_seeking_initial_weeks': Parameter( | ||
Types.INT, 'Weeks of gestation at which initial care seeking is applied'), | ||
'anc_scheduling_window': Parameter( | ||
Types.INT, 'Window in days for scheduling of ANC event'), |
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.
of first ANC event?
On you 'general' point above - we can delete any values which arent in the 'value' column. we can keep the references to specific studies however. Just going to look at your labelling of the parameters now |
@joehcollins thanks so much for taking the time to review this modifications. Super helpful, and really do agree with your recommendations and the push to rethink some of these parameter names to add clarity to the code. Going to go through your comments/recommendations in more detail. |
no worries! Could you share the criteria for labelling the parameters? I wanted to use that to review but couldnt find it - will do next week |
Yes, for the latest guidance, you can reference the tab 'Guide for Modelers' in this document HERE. Also, I'll be sure to tag you in the other labour modules that I've worked through that would also be helpful to start getting your eyes on. thanks! |
Parameters
Assumed
Variables that have been defined as 'assumed' in the write-up because no reliable data source identified for the parameter:
prob_ectopic_pregnancy
prob_mod_sev_aph
prob_glycaemic_control_diet_exercise
General
prob_ectopic_pregnancy
, the value column is populated with [0.01, 0.01] and the Primary Source (or calibration data) value is populated with [0.004937, 0.00366]. It seems that the values from Primary Source (or calibration data) are not used in the module?Local v. Universal
prob
labeled as local, and those with prefix rr, odds, and aor labeled as localtreatment_effect_ectopic_pregnancy_treatment
,treatment_effect_post_abortion_care
,prob_glycaemic_control_diet_exercise
,treatment_effect_gdm_case_management
,treatment_effect_still_birth_food_sups
), the rest (related to supplementation and medication) labeled as universal . Currently havetreatment_effect_still_birth_food_sups
labeled as local as I believe the type of food supplement may be location dependent.min_age_reproductive
&max_age_reproductive
: Defined as universaldeath_delay_days_after_abortion
&death_delay_days_after_ectopic_rupture
: both set to local because has to do with duration of time that treatment/case management can take into effect to prevent deathScenario
alternative_anc_coverage
andanc_availability_odds
which is the target that is set whenalternative_anc_coverage
is TRUE)Undetermined, Design Decision
hsi_event_window_days
: this parameter is used across multiple HSI events as it is it is the 'buffer' that is used for scheduling. To be discussed if helpful to have as this widely applicable parameter, or if each HSI event should have its own parameter (e.g.hsi_event_first_anc_appt_window_days
). I have opted to have them all fall under this singular parameter for readability + simplicityUndetermined, Calibration
prob_still_birth_per_month
: Is 'scaled at initiation' the same as calibrated?