Skip to content
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

Merged
merged 5 commits into from
Jul 13, 2024
Merged

Add more docs pages #554

merged 5 commits into from
Jul 13, 2024

Conversation

katy-sadowski
Copy link
Collaborator

@katy-sadowski katy-sadowski commented Jun 26, 2024

Adds:

  • measureValueCompleteness
  • measurePersonCompleteness
  • isStandardValidConcept
  • standardConceptRecordCompleteness
  • sourceConceptRecordCompleteness
  • sourceValueCompleteness
  • plausibleValueHigh
  • plausibleValueLow
  • withinVisitDates

Copy link
Collaborator

@MaximMoinat MaximMoinat left a 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.

vignettes/checks/measureValueCompleteness.Rmd Outdated Show resolved Hide resolved
vignettes/checks/measureValueCompleteness.Rmd Outdated Show resolved Hide resolved
vignettes/checks/measureValueCompleteness.Rmd Outdated Show resolved Hide resolved
vignettes/checks/measurePersonCompleteness.Rmd Outdated Show resolved Hide resolved
vignettes/checks/measurePersonCompleteness.Rmd Outdated Show resolved Hide resolved
vignettes/checks/standardConceptRecordCompleteness.Rmd Outdated Show resolved Hide resolved
vignettes/checks/standardConceptRecordCompleteness.Rmd Outdated Show resolved Hide resolved
- *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`.
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

Copy link
Collaborator

@MaximMoinat MaximMoinat Jul 11, 2024

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)

Copy link
Collaborator Author

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.

Copy link
Collaborator

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!!!

vignettes/checks/sourceConceptRecordCompleteness.Rmd Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jul 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.03%. Comparing base (3af4d8f) to head (a1f9a18).

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.
📢 Have feedback on the report? Share it here.

@katy-sadowski
Copy link
Collaborator Author

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!

@katy-sadowski katy-sadowski merged commit f2d64b7 into develop Jul 13, 2024
8 checks passed
@katy-sadowski katy-sadowski deleted the katy__docs branch July 13, 2024 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants