-
Notifications
You must be signed in to change notification settings - Fork 5
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
Consolidate CMD initial investigations #1028
Conversation
…ble (rather than returning an update), as this is more efficient
…D (as this is no longer covered inside the generic hsi)
…the same HSI (rather than one for each condition)
…to footprints that have blank footprints.
conditions_to_investigate.append(condition) | ||
|
||
if (not is_already_diagnosed) and has_symptom: | ||
has_any_symptoms = True |
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.
Am I right that "conditions_to_investigate" are to trigger investigation/diagnosis and treatment for the conditions (including hypertension), and that "has_any_symptoms" is to trigger investigation/diagnosis and treatment for hypertension separately? I wonder will these two trigger overlapping hsi usage for hypertension?
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, I noticed that line 223 "self.symptoms = {f"{s}_symptoms" for s in self.conditions if s != "hypertension"}" indicating that hypertension is not registered as a symptom; therefore, its "has_symptom" = False always? (If has_symptom = (f'{condition}_symptoms' in symptoms)) This separate route then will never make has_any_symptom = True thus no triggering for diagnosis or treatment for hypertension.
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.
Am I right that "conditions_to_investigate" are to trigger investigation/diagnosis and treatment for the conditions (including hypertension), and that "has_any_symptoms" is to trigger investigation/diagnosis and treatment for hypertension separately? I wonder will these two trigger overlapping hsi usage for hypertension
For one call to this function (for the person), a maximum of one instance of HSI_CardioMetabolicDisorders_Investigations
will be scheduled for that person. During that HSI, there is only one opportunity to trigger hypertension diagnosis/treatment and that is only for persons who have ANY of the relevant symptoms.
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, I noticed that line 223 "self.symptoms = {f"{s}_symptoms" for s in self.conditions if s != "hypertension"}" indicating that hypertension is not registered as a symptom; therefore, its "has_symptom" = False always? (If has_symptom = (f'{condition}_symptoms' in symptoms)) This separate route then will never make has_any_symptom = True thus no triggering for diagnosis or treatment for hypertension.
Yes, hypertension is not associated with a specific symptom. But a test for hypertension is launched if the person has ANY other symptom of cardiometabolic disorders (line 1502)
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.
(Am doing some renaming to improve the clarity here.)
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 Tim, for the great explanation and update. It is very clear now.
So just to confirm:
if for one person, hypertension is in conditions_to_investigate (e.g., the hypertension condition is due to test) and has_any_cmd_symptom = True (e.g., diabetes),
only one investigation HSI will be triggered by conditions_to_investigate, but
there will be two separate hypertension tests (and two treatments) following this HSI -> one is by do_for_each_condition function (line 1489), the other is by has_any_cmd_symptom condition (line 1505 onwards)?
If this repeating is not necessary, we might simply make the following changes (where the latter might be clearer and better)?
line 836 if (not is_already_diagnosed) and has_symptom and ('hypertension' not in conditions_to_investigate):
line 837 has_any_cmd_symptom = True
Or
line 1505 onwards
if (
self.has_any_cmd_symptom
and ('hypertension' not in conditions_to_investigate)
and (self.module.rng.rand() < self.module.parameters['hypertension_hsi']['pr_assessed_other_symptoms'])
):
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.
fixed in the new commits.
Hi Tim, I have read through the changes and relevant. It might be my limited understanding, but I did feel confused about the use of "has_symptom", compared to older logics, and thus made several comments there. And I wonder is it needed to create a test to for the function of "determine_if_will_be_investigated", or test "has_symptoms" (when True) in HSI_CardioMetabolicDisorders_Investigations. |
Many thanks @BinglingICL --- I think you spotted an important bug there, many thanks! |
Thanks for the further review @BinglingICL -- it's very helpful. I've also added some tests, as you suggest initially. I think left to do are:
|
Hi Tim, the tests are super clear and helpful. To check if I was wrong about the possible overlapping of hypertension treatment as raised above, I have tried the following test within test_hsi_following_presentation_at_generic_non_emergency_hsi (as in my last commit):
then, the assertion "assert len(conditions_being_treated) == len(set(conditions_being_treated))" failed because hypertension was treated twice. So it seems hypertension might be treated repeatedly for one person? |
I have made a second small commit trying to fix this possible issue. Please have a check and feel free to delete my commits if they are not necessary. |
Thanks for letting me know the following steps. |
Thanks for pointing that out. Hypertension was the one I was thinking of, as that is large and we did hear that this isn't done as routinely as guidelines say it should be. But, we will need to see if changing it affects the calibrations. I also noticed this passage below - which starts a stream of treatment for the person if they risk factors. But, it seems like there should be a probability to determine if it happens (that we can also put in the # If no follow-up treatment scheduled but the person has at least 2 risk factors, start weight loss treatment
# todo - add that this happens only with some probability on this?
if (
(not any(hsi_scheduled))
and (df.at[person_id, 'nc_risk_score'] >= 2)
and (not df.at[person_id, 'nc_ever_weight_loss_treatment'])
):
df.at[person_id, 'nc_ever_weight_loss_treatment'] = True
# Schedule a post-weight loss event for 6-12 months for individual to potentially lose weight:
self.sim.schedule_event(CardioMetabolicDisordersWeightLossEvent(self.module, person_id),
random_date(self.sim.date + pd.DateOffset(months=6),
self.sim.date + pd.DateOffset(months=12),
self.module.rng)) |
@BinglingICL -- you are 100% right about this: your addition to the test and the fix look absolutely right to me, and I had overlooked this subtle detail. Thanks very much |
scale-run at this point is |
Thanks Tim. As discussed, we will not do this but only to update the comment here, as the scheduled HSI is not about treatment. And I would like to approve this PR now as all my "little" concerns are solved. |
Yes, left to do on this one:
|
Consolidates HSI scheduled by CardioMetabolicDisorders to prevent many follow-up HSI being scheduled for the same person.