Skip to content

Create date_onset Property in cardio_metabolic_disorders.py#1849

Open
thewati wants to merge 25 commits intomasterfrom
wati/onset_in_cmd_module
Open

Create date_onset Property in cardio_metabolic_disorders.py#1849
thewati wants to merge 25 commits intomasterfrom
wati/onset_in_cmd_module

Conversation

@thewati
Copy link
Copy Markdown
Collaborator

@thewati thewati commented Mar 19, 2026

Creating date_onset property for conditions in the cardio_metabolic_disorders module. Fixes issue #1845. The nc_diabetes_date_onset property will be used in issue #1595

@thewati thewati requested review from mnjowe and tbhallett March 19, 2026 14:26
@thewati thewati self-assigned this Mar 19, 2026
@thewati thewati added the epi label Mar 19, 2026
@thewati
Copy link
Copy Markdown
Collaborator Author

thewati commented Mar 20, 2026

I thought it would be better to just capture the date_onset of all conditions. Then I could just access nc_diabetes_date_diagnosis in the Diabetic Retinopathy module later.
In the cardio_metabolic_disorders module, people may undergo remission or lose a condition through idx_loses_condition. Once this happens I am not resetting nc_diabetes_date_diagnosis because I thought preserving the historical onset date even after the condition resolves is important as it may still pose a risk

@thewati
Copy link
Copy Markdown
Collaborator Author

thewati commented Mar 20, 2026

Hi @mnjowe and @tbhallett,
This is ready for review

@mnjowe
Copy link
Copy Markdown
Collaborator

mnjowe commented Mar 23, 2026

This is well noted @thewati. I will take a look

@mnjowe
Copy link
Copy Markdown
Collaborator

mnjowe commented Mar 23, 2026

Hi @thewati. Looking at the use case of this PR, I think adding a property that captures date_onset of all condition will not be a good idea. The model is already heavy and I think we should exhaust all possible alternatives before we decide on introducing a new property. Also I think adding this date_onset property to all conditions while we are interested only in one condition will accumulate some unnecessary computational costs

In this case, how about we create a dictionary that maps diabetes individual Ids to their date of diabetes onset? This should only look into diabetes as we are not interested in any other conditions. That way, if you capture their date of onset in Diabetic Retinopathy you can delete the keys in the diabetes dictionary to make it as smaller as possible. What do you think?

@tbhallett any thoughts?

@thewati
Copy link
Copy Markdown
Collaborator Author

thewati commented Mar 24, 2026

Hi @thewati. Looking at the use case of this PR, I think adding a property that captures date_onset of all condition will not be a good idea. The model is already heavy and I think we should exhaust all possible alternatives before we decide on introducing a new property. Also I think adding this date_onset property to all conditions while we are interested only in one condition will accumulate some unnecessary computational costs

In this case, how about we create a dictionary that maps diabetes individual Ids to their date of diabetes onset? This should only look into diabetes as we are not interested in any other conditions. That way, if you capture their date of onset in Diabetic Retinopathy you can delete the keys in the diabetes dictionary to make it as smaller as possible. What do you think?

@tbhallett any thoughts?

Thanks for pointing this out. I had coded it this way so that anybody can use it in future if needed and also to maintain consistency... But I agree, this will cause unnecessary overhead. I can change this to a dictionary that only stores the date of onset as you suggested

@thewati
Copy link
Copy Markdown
Collaborator Author

thewati commented Apr 1, 2026

@mnjowe this is now using a dictionary. I'm done

@thewati thewati marked this pull request as ready for review April 7, 2026 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants