-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: function to join the LPR2 registers #118
Conversation
Co-authored-by: Signe Kirk Brødbæk <40836345+signekb@users.noreply.github.com>
…o-aarhus/osdc into feat/join-lpr2
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 nice! Looks good to me (except there's no overlap of recnum
in the simulated data - but that's a different issue).
Should this logic be added to algorithm.csv
as well?
Right now there is no logic for the algorithm here 😉 so no need to fill in the algorithm file. Only the |
Ah! I thought we wanted logic from all functions (including helper functions) in the |
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 a commit to expand a bit on the test data.
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.
Don't we need a recnum
in one of actual_lpr_diag
and/or actual_lpr_adm
that's not in the other to test that its actually a full join and not e.g., a left join?
In the real data there is always a record of each recnum in both _diag and _adm, so it doesn't matter if it's a full/inner/right or left-join in practice. Now that I think about it, I guess that if there was a mismatch/corruption in the input data, I think we would want to only keep the complete records, so I'll change it to inner_join? |
…into feat/join-lpr2
pnr = rep(1:2, 3), | ||
recnum = 2:7, | ||
c_spec = 1:6, | ||
d_inddto = c("20230101", "20220101", "20210101", "20200101", "20190101", "20180101"), |
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.
Is c_pattype
missing 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.
Um, c_pattype
isn't in the variable list.
Added some more tests and refined a few things, but nothing major. Merging in. |
Closes #88. Has tests and documentation generated.
I don't know what I did, but I think I made a branch off of the hba1c branch, so this is a stacked PR.