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

feat: function to join the LPR2 registers #118

Merged
merged 26 commits into from
Jun 25, 2024
Merged

feat: function to join the LPR2 registers #118

merged 26 commits into from
Jun 25, 2024

Conversation

lwjohnst86
Copy link
Member

@lwjohnst86 lwjohnst86 commented Jun 14, 2024

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.

Base automatically changed from feat/include-hba1c-criteria to main June 19, 2024 09:53
Copy link
Contributor

@signekb signekb left a 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?

@lwjohnst86
Copy link
Member Author

Right now there is no logic for the algorithm here 😉 so no need to fill in the algorithm file. Only the include_ and exclude_ functions contain the actual logic. This and the other future functions are helper functions.

@signekb
Copy link
Contributor

signekb commented Jun 19, 2024

Ah! I thought we wanted logic from all functions (including helper functions) in the algorithms.csv

Copy link
Collaborator

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

Copy link
Contributor

@signekb signekb left a 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?

@Aastedet
Copy link
Collaborator

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?

pnr = rep(1:2, 3),
recnum = 2:7,
c_spec = 1:6,
d_inddto = c("20230101", "20220101", "20210101", "20200101", "20190101", "20180101"),
Copy link
Contributor

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?

Copy link
Member Author

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.

@lwjohnst86
Copy link
Member Author

Added some more tests and refined a few things, but nothing major. Merging in.

@lwjohnst86 lwjohnst86 merged commit 91eaacc into main Jun 25, 2024
1 check failed
@lwjohnst86 lwjohnst86 deleted the feat/join-lpr2 branch June 25, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Create function: join_lpr2()
3 participants