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.

Sign up for GitHub

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

Jump to bottom

docs: 📝 expand on inclusions and exclusions #133

Merged
merged 33 commits into from
Dec 19, 2024
Merged

docs: 📝 expand on inclusions and exclusions #133

merged 33 commits into from
Dec 19, 2024

Conversation

Aastedet
Copy link
Collaborator

@Aastedet Aastedet commented Sep 18, 2024 •
edited by signekb
Loading

ChatGPT seems to understand what I'm trying to describe (although it shouldn't apply the exclusion functions just yet): https://chatgpt.com/share/66eab6f4-0ffc-800a-aedb-d9dae21c48c3 😄

Closes #130
Closes #140

signekb
signekb reviewed Sep 18, 2024
View reviewed changes
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
@Aastedet Aastedet marked this pull request as ready for review September 19, 2024 11:40
@lwjohnst86 lwjohnst86 changed the title Fleshed out and updated include_gld_purchases() flow documentation docs: 📝 expand on inclusions and exclusions Sep 19, 2024
@Aastedet @signekb
Update vignettes/function-flow.Rmd
f03a4da 
Co-authored-by: Signe Kirk Brødbæk <40836345+signekb@users.noreply.github.com>
@Aastedet
Copy link
Collaborator Author

Since the join_lpr2() and join_lpr3() functions already perform variable selection, we should also include a baseline filtering operation, since that is what we would do in practice to avoid processing unnecessary data: load-> select variables -> filter to only diagnoses relevant to the osdc package (similar to how the include_gld_purchases() function filters to A10*).

Can we do this as-is (my uneducation opinion), or do we need to change the function name?

@lwjohnst86
Copy link
Member

@Aastedet it sounds like we need some processing before joining then. Could you document those filtering steps in the function flow/design docs?

lwjohnst86
lwjohnst86 reviewed Sep 19, 2024
View reviewed changes
Copy link
Member

@lwjohnst86 lwjohnst86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is coming along nicely!! Just some comments for now.

vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
@Aastedet
Copy link
Collaborator Author

@Aastedet it sounds like we need some processing before joining then. Could you document those filtering steps in the function flow/design docs?

Yes - I've added them in the latest commit

lwjohnst86
lwjohnst86 reviewed Sep 20, 2024
View reviewed changes
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
lwjohnst86
lwjohnst86 reviewed Sep 20, 2024
View reviewed changes
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
@lwjohnst86 lwjohnst86 mentioned this pull request Sep 20, 2024
Merged
lwjohnst86 added a commit that referenced this pull request Sep 20, 2024
@lwjohnst86
refactor: ♻️ function to create LPR2
0ad86d1 
Based on updates from #133
@lwjohnst86 lwjohnst86 mentioned this pull request Sep 20, 2024
Draft
Anders Aasted Isaksen added 3 commits September 27, 2024 12:05
Added correct diagnoses to filter to in lpr_join() functions.
7cca920 
Reformatted output variable lists to start with variable names, followed by a short description. Also added info on renamed variables.
changed specialty values to align with the PR with a refactored creat…
b40412c 
…e_lpr2() function
Joining inclusions and definition. Looking to add type classification.
35118e8
@Aastedet
Copy link
Collaborator Author

Aastedet commented Dec 16, 2024 •
edited
Loading

The first draft describing the function flow is done now. Some minor details probably need aligning through-out the document for consistency, and some of the function flow might not be the best way to technically implement the pipeline, but let's discuss at the meeting later.

@Aastedet Aastedet requested a review from signekb December 16, 2024 23:45
Anders Aasted Isaksen added 2 commits December 17, 2024 20:56
Removed helper function for dropping first event as it seemed a bit e…
cbee21f 
…xcessive.

Added a bit more detail to the raw_inclusion_date vs stable_inclusion_date.
Added function flow description of get_diabetes_type() and its helper…
3820459 
… functions.
@Aastedet Aastedet requested a review from lwjohnst86 December 17, 2024 22:59
@Aastedet Aastedet mentioned this pull request Dec 18, 2024
Open
signekb
signekb requested changes Dec 18, 2024
View reviewed changes
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.

Nice, Anders! 👍 I have some suggestions + questions.

vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
lwjohnst86
lwjohnst86 requested changes Dec 18, 2024
View reviewed changes
Copy link
Member

@lwjohnst86 lwjohnst86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking this over, I can feel a bit overwhelmed with everything. And I think it's because there is some mixing of the details of the algorithm with the interfaces of the functions we want to have.

I think it would help if there was a clearer separation between the two: algorithm implementation and (internal) function interfaces. For instance, the algorithm details can go into filtering, excluding, keeping, etc while the function interface can focus on what are the arguments, what are the input types for the arguments, and what is the output.

I will make a PR to stack on this one to see if I can split it out a bit more.

lwjohnst86 added a commit that referenced this pull request Dec 18, 2024
@lwjohnst86
docs: 🏗️ LPR2 creation as own diagram (#151)
95cabfe 
There were too many things going on with this so I split out it. This is
based on the changes @Aastedet did in #133
lwjohnst86 and others added 3 commits December 18, 2024 17:50
@lwjohnst86 @signekb
docs: ✏️ small edits from review
d294071 
Co-authored-by: Signe Kirk Brødbæk <40836345+signekb@users.noreply.github.com>
@lwjohnst86
Merge branch 'main' of https://github.com/steno-aarhus/osdc into upda…
159ab16 
…te-function-flow

# Conflicts:
#	vignettes/function-flow.Rmd
@lwjohnst86
Merge branch 'update-function-flow' of https://github.com/steno-aarhu…
b94e36d 
…s/osdc into update-function-flow
This was referenced Dec 18, 2024
Closed
Draft
lwjohnst86
lwjohnst86 reviewed Dec 19, 2024
View reviewed changes
Copy link
Member

@lwjohnst86 lwjohnst86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ops forgot to submit these comments. Anyway, super great job @Aastedet, it's really helpful! I've been converting them over into pseudocode/algorithm-data in #163

vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
vignettes/function-flow.Rmd Outdated Show resolved Hide resolved
@Aastedet @signekb
Update vignettes/function-flow.Rmd
da55507 
get_diagnosis_date() renamed to get_inclusion_date()

Co-authored-by: Signe Kirk Brødbæk <40836345+signekb@users.noreply.github.com>
lwjohnst86
lwjohnst86 approved these changes Dec 19, 2024
View reviewed changes
Copy link
Member

@lwjohnst86 lwjohnst86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to merge this in so the stacked PR can be synched with main

@Aastedet Aastedet mentioned this pull request Dec 19, 2024
Closed
@lwjohnst86 lwjohnst86 merged commit 75346cc into main Dec 19, 2024
1 check passed
@lwjohnst86 lwjohnst86 deleted the update-function-flow branch December 19, 2024 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@signekb signekb signekb requested changes

@lwjohnst86 lwjohnst86 lwjohnst86 approved these changes

Assignees

@Aastedet Aastedet

Labels
None yet
Projects
osdc development planning board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Document join() functions Go through the function flow vignette and fill in missing function info
3 participants
@Aastedet @lwjohnst86 @signekb