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

Rewrite temporalPlausibleAfter for checking events before birth #445

Closed
MaximMoinat opened this issue May 1, 2023 · 3 comments · Fixed by #516
Closed

Rewrite temporalPlausibleAfter for checking events before birth #445

MaximMoinat opened this issue May 1, 2023 · 3 comments · Fixed by #516
Assignees
Labels
enhancement New feature or request

Comments

@MaximMoinat
Copy link
Collaborator

The check temporalPlausibleAfter is used to check for dates before birth based on PERSON.BIRTH_DATETIME.
However, this is a non-required field and therefore will be empty in many databases.

I would propose to use PERSON.YEAR_OF_BIRTH by default, for which there is already logic in field_plausible_temporal_after.sql. To make this work, the plausibleTemporalAfterTableName is set to PERSON and plausibleTemporalAfterFieldName has to be left empty in the field-level thresholds file.

@MaximMoinat MaximMoinat added new check New DQ check to be added and removed new check New DQ check to be added labels May 1, 2023
@MaximMoinat
Copy link
Collaborator Author

MaximMoinat commented May 1, 2023

Another note: the logic uses June 1st is used as a proxy together with the year of birth. This is tricky, as half of the persons will be born before that.

We could combine all four date of birth fields into one query, using the information that is available and defaulting to the first day of the month/year when unavailable.

COALESCE(
    plausibleTable.birth_datetime, 
    CAST(CONCAT(
        plausibleTable.year_of_birth, '-',
        COALESCE(plausibleTable.month_of_birth, 1), '-',
        COALESCE(plausibleTable.day_of_birth, 1)
     ) AS DATE)
)

To replace the coalesce in:

{'@plausibleTemporalAfterTableName' == 'PERSON'}?{
COALESCE(
CAST(plausibleTable.@plausibleTemporalAfterFieldName AS DATE),
CAST(CONCAT(plausibleTable.year_of_birth,'-06-01') AS DATE)
)
}:{

This might warrant a whole new check specifically checking for events before birth (akin to plausibleDuringLife that checks for events after death).

@MaximMoinat MaximMoinat changed the title Use year of birth as default field for temporalPlausibleAfter to check events before birth Rewrite temporalPlausibleAfter for checking events before birth May 1, 2023
@katy-sadowski
Copy link
Collaborator

I agree with the proposal here! Also agree that comparisons to DOB may be better off in their own check to reduce complexity of temporalPlausibleAfter ... I was just thinking we could add these into plausibleDuringLife instead and then realized that's maybe what you're up to in #454? If yes - I support it!

@MaximMoinat
Copy link
Collaborator Author

Thanks Katy. I will indeed add this to the PR you mentioned, refactoring the plausibleDuringLife check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants