-
Notifications
You must be signed in to change notification settings - Fork 97
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
check gender specific events - descendants of a given concept #519
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #519 +/- ##
=======================================
Coverage 86.42% 86.42%
=======================================
Files 16 16
Lines 884 884
=======================================
Hits 764 764
Misses 120 120 ☔ View full report in Codecov by Sentry. |
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 this @dimshitc ! Overall looks good but I have a couple of questions:
- What are your thoughts on flipping this around so that the denominator is # persons with the gender, and numerator is # persons w/ an implausible concept for that gender? I think that doing so would make more sense from a database-wide data quality perspective. But need to ponder this more - this could be a good one to discuss again at our next DQ meeting
- Do the ancestor concepts we use here subsume all of the other individual concepts in the threshold file? If yes - I'm thinking that we could deprecate the old plausibleGender checks the same way Maxim did for temporal plausibility checks here: https://github.com/OHDSI/DataQualityDashboard/pull/516/files# (with the idea to eventually get rid of them altogether). If they don't subsume all of the other concepts, maybe we could add a couple more ancestor rows to get the rest of them?
- I noticed we have Procedure on female genital system ancestor but not the same for males, should we add it?
Final thing - please change the target branch to develop
. (You may need to rebase your branch if you pulled from main originally).
Thanks Katy!
2.1. conditions
|
Ok, when I rebase to the develop it shows 37 files changed, so it becomes hard to track changes, I suggest we agree on all the changes, and then I create the new branch from the develop, with the latest changes |
@dimshitc Sounds good for those additional concepts, and for leaving off male procedures for now. For 444094 I did a quick check of the ICD10-CMs and there is only one P-code (perinatal record code) in there, which is probably a mis-mapping? Not sure about other vocabs, but assume/hope that this indicates 444094 should only include findings on the mother? Also agree for starting a fresh branch off develop rather than dealing with the main<>develop diff. Maybe you can put the changes from your other new check PR on there too, and we can merge them in together? :) Look forward to chatting about the denominator in the meeting Thurs. It's unideal both ways, but now I see how it could end up being a really small failure % and hard to make a threshold for, if we make the denominator persons. |
This adds new check when a concept set of gender specific concepts is evaluated - whether patients with these events have a correct gender.
Changes were made only in CDM v54, if approved, I'll add them to the other CDM versions.
Currently this check looks similar to the checks without descendant usage (In the image above I had to filter by description including 'descendants'). Please let me know if we need to distinguish it more.