-
Notifications
You must be signed in to change notification settings - Fork 99
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
Add new per-check documentation pages and check severity parameter #518
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #518 +/- ##
===========================================
- Coverage 85.00% 84.59% -0.41%
===========================================
Files 16 16
Lines 907 909 +2
===========================================
- Hits 771 769 -2
- Misses 136 140 +4 ☔ 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.
This looks great, thank you Katy for putting these together!
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.
Very nice work. I only have some small suggestions for the template, see inline comments on the fkClass page.
@katy-sadowski I have applied a new markdown template to the existing checks, and instantiated the template for all other checks as well.
|
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.
Thank you so much for these additions @MaximMoinat ! The template looks great and is super useful. I left some minor suggestions on the plausible after birth doc.
As mentioned in Teams I will wait to merge this until @dimshitc 's new checks are merged in, to save him merge conflict grief 😄
**Context**: Verification\ | ||
**Category**: Plausibility\ | ||
**Subcategory**: Temporal\ | ||
**Severity**: Characteristic |
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 we were calling this "Characterization"?
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 put emojis next to the other 2 statuses (a skull for fatal and a warning sign for CDM convention). feel free to choose and add one for characterization 😄
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.
So about the emojis. I looked for one but did not find a good one. I added a ✔
✔. But I am not really convinced about the need for them for this purpose (although in general I love using emojis everywhere). Let's discuss next meeting.
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.
Haha, they are definitely not necessary. I was thinking maybe in the new report view we build eventually, having some visual marker for the severity could be helpful, but that's something to be discussed later on as we think through the UI.
|
||
- Aggressive: Remove all patients who have at least one record before birth (if the birthdate of this patient is unreliable) | ||
- Less aggressive: Remove all rows that happen before birth. Probably this should be chosen as a conventional approach for data clean up (if the event dates are unreliable) | ||
- Conservative: set event date to birth date. |
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.
maybe should also add the option of leaving as-is, i.e. for prenatal or other observations where the date is actually valid.
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 actually like that better for the conservative strategy. Modifying the event date might create other issues. Either it is correct and you leave it as is, or you don't trust the record and remove it, or you don't trust the person and remove it.
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.
@MaximMoinat thanks for the fixes! Just 2 more minor things.
c79fd12
to
9c166f8
Compare
This pull request contains the following changes:
severity
parameter is added to the check descriptions file and to check documentation, as decided in the Fall 2023 DQD Hackathon. This parameter is not used for anything yet, but in the future we will add the ability to specify which severity level(s) to execute in a DQD runNotes for reviewers:
pkgdown::build_site()
so you can review the actual webpage rather than the markdown files