-
Notifications
You must be signed in to change notification settings - Fork 18
Vignettes for dm and ae domains #120
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
base: main
Are you sure you want to change the base?
Conversation
rammprasad
left a comment
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.
GOod job,. Looks awesome. Minor changes.
| ) | ||
| ``` | ||
|
|
||
| ## Create reference dates configuration file {#refdates} |
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.
Can you add the column definitions here? It will be easier to the user to understand. Ideally, the user should understand how to prepare this file by looking at this Vignette
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.
Added definitions of each column
vignettes/demographics_domain.Rmd
Outdated
| dm <- | ||
| # Map AGE using assign_no_ct | ||
| assign_no_ct( | ||
| raw_dat = dm_raw, |
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 am gettting a CT error while execuring this chunk. Is it working for you?
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 works fine for me. Could you double check if the study_ct file you loaded contains 138 observations?
vignettes/demographics_domain.Rmd
Outdated
| # Map DTHDTC using oak_cal_ref_dates | ||
| oak_cal_ref_dates(ds_in = ., | ||
| der_var = "DTHDTC", | ||
| min_max = "max", |
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 for death date, it has to be min.
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.
updated
vignettes/demographics_domain.Rmd
Outdated
| # Derive RFSTDTC using oak_cal_ref_dates | ||
| oak_cal_ref_dates(ds_in = ., | ||
| der_var = "RFSTDTC", | ||
| min_max = "min", |
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 will be beneficial to explain how to choose min or max for each function call. Explain the derivation of RFSTDTC and explain the parameters, like min or max. Lets just do it for one min and one max examples and then rest doesn't need 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.
I added separate descriptions for RFSTDTC and RFENDTC explaining the selection of min/max. Please review again and see how it looks to you
… into 116-docs-aedm
Merge remote-tracking branch 'origin/main' into 116-docs-aedm # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
closes #116
study_ctfile