Add three new temporal checks: `plausibleAfterBirth`, `plausibleBeforeDeath`, `plausibleStartBeforeEnd` #516

MaximMoinat · 2023-12-21T07:39:03Z

As discussed in #454: these checks separate out the quality checks in plausibleDuringLife and plausibleTemportalAfter.

plausibleAfterBirth checks for any events happening before birth (was implicitly checked in plausibleTemportalAfter)
plausibleBeforeDeath checks for any events happening after death (improving upon implementation of plausibleDuringLife)
plausibleStartBeforeEnd checks for end dates that are before start date in the same table (specific implementation of plausibleTemportalAfter)

TODO:

Add threshold settings for v5.2 and v5.3 (after validating that this works for v5.4)
To discuss: disable plausibleDuringLife and plausibleTemportalAfter by default. These are redundant if the three new checks are run, but kept in for backwards compatibility.

fixes #513 and fixes #445

codecov · 2023-12-21T07:44:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2883027) 86.42% compared to head (cdb6db7) 86.48%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #516      +/-   ##
===========================================
+ Coverage    86.42%   86.48%   +0.06%     
===========================================
  Files           16       16              
  Lines          884      888       +4     
===========================================
+ Hits           764      768       +4     
  Misses         120      120

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dimshitc

why do we keep plausibleTemporalAfter and plausibleDuringLife checks?
aren't they covered now by these new 3 checks?

MaximMoinat · 2023-12-21T14:04:34Z

why do we keep plausibleTemporalAfter and plausibleDuringLife checks? aren't they covered now by these new 3 checks?

Removing the 'replaced' checks might be a breaking change for some people (e.g. when comparing number of violating records over time). My thinking was to implement a 'soft' disable, making it possible to execute the old checks if desired. e.g. by removing the plausibleDuringLife and plausibleTemportalAfter checks from the checkDescriptions object here, and only keep them if specified specifically in the checkNames parameter.

MaximMoinat · 2023-12-21T14:09:46Z

R/executeDqChecks.R

@@ -252,6 +253,10 @@ executeDqChecks <- function(connectionDetails,

  if (length(checkNames) > 0) {
    checkDescriptionsDf <- checkDescriptionsDf[checkDescriptionsDf$checkName %in% checkNames, ]
+  } else {
+    # No checkNames specified. Do not run plausibleDuringLife and plausibleTemportalAfter as these are superseded three new checks


@dimshitc This disables the old checks, if user did not specify them specifically. We do need to document this behaviour.

This makes sense to me.

Should we set threshold in all these checks to 0? as these are obvious mistakes and shouldn't be in the data.

I've been thinking this over. I think we should actually enable all of the checks (new & old) by default, and instruct users how to disable the ones they don't want in the documentation. Otherwise this is essentially a breaking change because users relying on the old behavior will need to take action to adapt to the change. We can also add a warning that raises when the 2 old checks are enabled to alert users that the new checks are preferred.

Does that sound OK to you guys?

Yes, that makes even more sense. I added the following warnings if one of the old checks is run:
"DEPRECATION WARNING - The plausibleDuringLife check has been reimplemented with the plausibleBeforeDeath check."

DEPRECATION WARNING - The plausibleTemporalAfter check has been reimplemented with the plausibleAfterBirth and plausibleStartBeforeEnd checks.

Regarding disabling the checks by the user; there is not really a nice solution as executeDqChecks only takes the names of checks to be run, not an argument with checks to exclude. So the user needs to list all checks except these two. Or does one of you see a more elegant solution?

Warnings look great; thanks!

And I wouldn't call it elegant, but I did add these instructions to the docs earlier this year on how to exclude a list of checks:

DataQualityDashboard/vignettes/DataQualityDashboard.rmd

Lines 114 to 120 in 81230bd

# want to EXCLUDE a pre-specified list of checks? run the following code:

#

# checksToExclude <- c() # Names of check types to exclude from your DQD run

# allChecks <- DataQualityDashboard::listDqChecks()

# checkNames <- allChecks$checkDescriptions %>%

# subset(!(checkName %in% checksToExclude)) %>%

# select(checkName)

I think for now we can point to this in the release notes. In the future I'd love to improve the workflow for enabling/disabling checks and editing thresholds (a la #253 ).

katy-sadowski

Thanks so much for this @MaximMoinat ! I left a couple suggestions.

katy-sadowski · 2023-12-24T19:57:14Z

R/executeDqChecks.R

@@ -252,6 +253,10 @@ executeDqChecks <- function(connectionDetails,

  if (length(checkNames) > 0) {
    checkDescriptionsDf <- checkDescriptionsDf[checkDescriptionsDf$checkName %in% checkNames, ]
+  } else {
+    # No checkNames specified. Do not run plausibleDuringLife and plausibleTemportalAfter as these are superseded three new checks


I've been thinking this over. I think we should actually enable all of the checks (new & old) by default, and instruct users how to disable the ones they don't want in the documentation. Otherwise this is essentially a breaking change because users relying on the old behavior will need to take action to adapt to the change. We can also add a warning that raises when the 2 old checks are enabled to alert users that the new checks are preferred.

Does that sound OK to you guys?

katy-sadowski · 2023-12-24T20:03:02Z

inst/sql/sql_server/field_plausible_after_birth.sql

+                    p.year_of_birth, '-',
+                    COALESCE(p.month_of_birth, 1), '-',
+                    COALESCE(p.day_of_birth, 1)


I think we will actually want to use the date format YYYYMMDD here to ensure we're compatible with all SQL dialects (see this thread here where I was discussing this with Martijn).

Good catch, I actually came across a similar issues in sqlite.

Not a trivial change, but I came up with the following code, which is a bit complex but should be compatible with all Sql dialects:

CAST(CONCAT( p.year_of_birth, COALESCE( RIGHT('00' + CAST(p.month_of_birth AS VARCHAR), 2), '01' ), COALESCE( RIGHT('00' + CAST(p.day_of_birth AS VARCHAR), 2), '01' ) ) AS DATE)

hah, crafty little solution! only question - do we need 00 or just 0 there? (won't actually impact the result)

Well spotted, will change it

katy-sadowski · 2023-12-24T20:05:09Z

inst/sql/sql_server/field_plausible_after_birth.sql

+            AND c.COHORT_DEFINITION_ID = @cohortDefinitionId
+        }
+        JOIN @cdmDatabaseSchema.person p ON cdmTable.person_id = p.person_id
+        WHERE CAST(cdmTable.@cdmFieldName AS DATE) < COALESCE(


i think to be safe (here and in other queries) we should also have a condition that the field is non-null.

also what do you think about adding that condition to the denominator as well? i.e. among all records where we have a date, that date must be plausible.

Agree, added same condition for the other two checks as well

inst/sql/sql_server/field_plausible_before_death.sql

katy-sadowski

OK I promise this is my last review 😅 I did a deep review and left a couple more comments on the SQL comments. I also noticed the following in the threshold files (I only checked 5.4 but assume they're all the same):

EPISODE start_date and start_datetime are missing a threshold for plausibleStartBeforeEnd
The plausibleStartBeforeEnd threshold for OBSERVATION_PERIOD is 1. Should this actually be 0, since this is a derived table and folks can control it in ETL (and also would prob cause some pretty weird stuff in analytics if the dates were flipped)?
plausibleAfterBirth and plausibleBeforeDeath need to be activated for MEASUREMENT.measurement_datetime

inst/sql/sql_server/field_plausible_after_birth.sql

inst/sql/sql_server/field_plausible_start_before_end.sql

inst/sql/sql_server/field_plausible_after_birth.sql

inst/sql/sql_server/field_plausible_before_death.sql

inst/sql/sql_server/field_plausible_start_before_end.sql

…o issue-445

MaximMoinat · 2024-01-05T11:34:12Z

Thanks @katy-sadowski for this thorough review, I agree with all your comments and adjusted the code accordingly.

katy-sadowski

Looks great! Thanks so much @MaximMoinat! I will go ahead and merge this into develop.

MaximMoinat added 3 commits December 20, 2023 15:42

add sql for new temporal plausibility checks

4c98f19

add v5.4 thresholds for new checks

efd4691

sql fixes

a2fa77d

MaximMoinat added the new check New DQ check to be added label Dec 21, 2023

MaximMoinat requested a review from dimshitc December 21, 2023 07:39

MaximMoinat mentioned this pull request Dec 21, 2023

Redefine field_plausible_during_life and field_plausible_temporal_after #454

Closed

dimshitc reviewed Dec 21, 2023

View reviewed changes

MaximMoinat added 2 commits December 21, 2023 15:04

update v5.2 and v5.3 thresholds

a525dd7

disable plausibleDuringLife and plausibleTemportalAfter

1807cce

MaximMoinat commented Dec 21, 2023

View reviewed changes

katy-sadowski requested changes Dec 24, 2023

View reviewed changes

MaximMoinat added 3 commits December 28, 2023 13:56

add deprecation warnings and documentation

313ba38

fix plausible after birth date

1d33ed5

violating field should be non-null

72b2f5b

MaximMoinat requested review from katy-sadowski and dimshitc December 28, 2023 13:30

Update field_plausible_after_birth.sql

0b4734f

katy-sadowski reviewed Jan 3, 2024

View reviewed changes

inst/sql/sql_server/field_plausible_before_death.sql Outdated Show resolved Hide resolved

MaximMoinat commented Jan 3, 2024

View reviewed changes

inst/sql/sql_server/field_plausible_before_death.sql Outdated Show resolved Hide resolved

fix typo

9c43666

katy-sadowski requested changes Jan 5, 2024

View reviewed changes

MaximMoinat added 2 commits January 5, 2024 12:32

fixes after review

dccea24

Merge branch 'issue-445' of github.com:OHDSI/DataQualityDashboard int…

cdb6db7

…o issue-445

katy-sadowski approved these changes Jan 6, 2024

View reviewed changes

katy-sadowski merged commit 1c48fcb into develop Jan 6, 2024
10 checks passed

MaximMoinat deleted the issue-445 branch May 10, 2024 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add three new temporal checks: `plausibleAfterBirth`, `plausibleBeforeDeath`, `plausibleStartBeforeEnd` #516

Add three new temporal checks: `plausibleAfterBirth`, `plausibleBeforeDeath`, `plausibleStartBeforeEnd` #516

MaximMoinat commented Dec 21, 2023 •

edited

Loading

codecov bot commented Dec 21, 2023 •

edited

Loading

dimshitc left a comment

MaximMoinat commented Dec 21, 2023

MaximMoinat Dec 21, 2023

dimshitc Dec 22, 2023

katy-sadowski Dec 24, 2023

MaximMoinat Dec 28, 2023

MaximMoinat Dec 28, 2023

katy-sadowski Dec 30, 2023

katy-sadowski left a comment

katy-sadowski Dec 24, 2023

katy-sadowski Dec 24, 2023

MaximMoinat Dec 28, 2023

katy-sadowski Dec 30, 2023

MaximMoinat Jan 2, 2024

katy-sadowski Dec 24, 2023

MaximMoinat Dec 28, 2023

katy-sadowski left a comment

MaximMoinat commented Jan 5, 2024

katy-sadowski left a comment

	# want to EXCLUDE a pre-specified list of checks? run the following code:
	#
	# checksToExclude <- c() # Names of check types to exclude from your DQD run
	# allChecks <- DataQualityDashboard::listDqChecks()
	# checkNames <- allChecks$checkDescriptions %>%
	# subset(!(checkName %in% checksToExclude)) %>%
	# select(checkName)

Add three new temporal checks: plausibleAfterBirth, plausibleBeforeDeath, plausibleStartBeforeEnd #516

Add three new temporal checks: plausibleAfterBirth, plausibleBeforeDeath, plausibleStartBeforeEnd #516

Conversation

MaximMoinat commented Dec 21, 2023 • edited Loading

codecov bot commented Dec 21, 2023 • edited Loading

Codecov Report

dimshitc left a comment

Choose a reason for hiding this comment

MaximMoinat commented Dec 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katy-sadowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katy-sadowski left a comment

Choose a reason for hiding this comment

MaximMoinat commented Jan 5, 2024

katy-sadowski left a comment

Choose a reason for hiding this comment

Add three new temporal checks: `plausibleAfterBirth`, `plausibleBeforeDeath`, `plausibleStartBeforeEnd` #516

Add three new temporal checks: `plausibleAfterBirth`, `plausibleBeforeDeath`, `plausibleStartBeforeEnd` #516

MaximMoinat commented Dec 21, 2023 •

edited

Loading

codecov bot commented Dec 21, 2023 •

edited

Loading