-
Notifications
You must be signed in to change notification settings - Fork 93
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 more docs pages #554
Add more docs pages #554
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.
Very complete. Could not help myself to make a number of suggestions. Did not yet review the last four (sourceValueCompleteness, plausibleLow/High and withinVisitDates).
Also, will add a commit to add % for all thresholds, to be consistent with the existing documentation pages. There are some other consistency things as well we might want to look at eventually (use of ``
or table/field names, capitalisation, use of @cdmTable
/@cdmField
, query formatting, field vs column, record vs row).
Lastly want to note that, while reading through, the descriptions quickly become quite abstract. Examples might help, something for a next version.
- *CDM Fields/Tables*: | ||
- *Default Threshold Value*: | ||
- *Numerator*: The number of rows with a value of 0 in the `X_source_concept_id` source concept field. In the case of `MEASUREMENT.unit_source_concept_id` and `OBSERVATION.unit_source_concept_id`, the number of rows with a value of 0 in the `X_source_concept_id` source concept field AND a non-NULL `value_as_number`. | ||
- *Denominator*: The total number of rows in the table. In the case of `MEASUREMENT.unit_source_concept_id` and `OBSERVATION.unit_source_concept_id`, the number of rows with non-NULL `value_as_number`. |
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.
It might be because it is late, but I can't find OBSERVATION.unit_source_concept_id
. Only Measurement and Device Exposure have a unit_source_concept_id.
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.
whoops, that was my error - will update to reference device instead of observation 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.
I think the misunderstanding starts with that we use the same query for both standard and source concept id, with the same exception for unit concepts
WHERE cdmTable.@cdmFieldName = 0 {@cdmFieldName == 'UNIT_CONCEPT_ID' & (@cdmTableName == 'MEASUREMENT' | @cdmTableName == 'OBSERVATION')}?{AND cdmTable.value_as_number IS NOT NULL} |
Note that we don't have this exception for the device table (unit_concept_id
nor unit_source_concept_id
) as it does not have a value_as_number
. I assume it refers to the quantity
field. Same for specimen table.
So we actually need to update the query to be consistent. It becomes a bit messy, but this would be the new where-clause.
WHERE cdmTable.@cdmFieldName = 0
{@cdmTableName IN ('MEASUREMENT', 'OBSERVATION') & @cdmFieldName IN ('UNIT_CONCEPT_ID', 'UNIT_SOURCE_CONCEPT_ID')}?{AND cdmTable.value_as_number IS NOT NULL}
{@cdmTableName IN ('DEVICE_EXPOSURE', 'SPECIMEN') & @cdmFieldName IN ('UNIT_CONCEPT_ID', 'UNIT_SOURCE_CONCEPT_ID')}?{AND cdmTable.quantity IS NOT NULL}
That said, after thinking this through for a bit, if the ETL is properly defined it should not be an issue (i.e. if value is empty, then also unit should be empty)
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.
Oh my! Great catch; we don't even check this on source concept ID fields, or any field in device or specimen. So for now I will update this to remove the language altogether for source concept ID. I'll file an issue to get this updated.
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.
@katy-sadowski Opened the issue here: #558
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!!!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #554 +/- ##
========================================
Coverage 84.03% 84.03%
========================================
Files 16 16
Lines 921 921
========================================
Hits 774 774
Misses 147 147 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Maxim Moinat <maximmoinat@gmail.com>
Thank you SO much for your review @MaximMoinat ! Your feedback and suggestions were extremely useful. I will plan to merge this in tomorrow, in case you want to take a final look. Then, I will merge in your other doc PR (#557), and then I will do the release! |
Adds: