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

feat: ✨ add include_diabetes_diagnoses() with tests #192

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

feat: ✨ add include_diabetes_diagnoses() with tests #192

wants to merge 10 commits into from

Conversation

Aastedet
Copy link
Collaborator

@Aastedet Aastedet commented Feb 6, 2025 •
edited
Loading

This PR adds:

  • The logic for filtering diabetes diagnoses in lpr2 and lpr3 to algorithm.csv
  • The include_diabetes_diagnoses() function + tests.
    • I changed the way department specialty is defined in LPR3 from exactly matching a string vector to be regex-based instead. It's not 100% identical to the previous implementation, but it doesn't have to be, as it wasn't a gold standard, and most of the departments specialties are edge cases anyway (type-specific diabetes primary diagnoses mainly come from a few specialties). This way is more succinct and should be more robust to register data shenanigans.
    • The data in the tibbles for testing I've added might not cover 100% of all edge cases, but it's a good start at least. I'm not sure if the included tests are the best implementation, but they somewhat cover my eyeballing of the output.
  • A helper function, get_majority_t1d_diagnoses(), containing the logic defining majority of diabetes diagnoses + tests for this function

Closes #91

Checklist

  • Haven't just run-all

Aastedet and others added 8 commits February 6, 2025 14:23
@Aastedet
chore: 🍱 re-generate roxygen2 documentation
3dee8dd
@Aastedet
Added helper function: get_majority_t1d_diagnoses
c573620 
Finished include_diabetes_diagnoses function.
@Aastedet
Merge branch 'include-diabetes-diagnoses' of https://github.com/steno…
4af2493 
…-aarhus/osdc into include-diabetes-diagnoses
@Aastedet
chore: 🍱 re-generate roxygen2 documentation
fd51a17
@Aastedet
Added tests for get_majority_t1d_diagnoses()
aff4434 
Minor edits to the function itself so handle missing values properly.
@Aastedet
Merge branch 'include-diabetes-diagnoses' of https://github.com/steno…
cff5f0b 
…-aarhus/osdc into include-diabetes-diagnoses
@Aastedet
Updated get_majority_t1d_diagnoses. It now passed all tests!
ff99192 
Slightly reworded some error messages.
@Aastedet
Added tests for include_diabetes_diagnoses(). Passes all tests!
80ff92a 
Edits to include_diabetes_diagnoses to fix logic for counting diabetes diagnoses
@Aastedet Aastedet changed the title Include diabetes diagnoses feat: 🚧 Include diabetes diagnoses Feb 7, 2025
@Aastedet Aastedet changed the title feat: 🚧 Include diabetes diagnoses feat: ✨ add include_diabetes_diagnoses() with tests Feb 7, 2025
@Aastedet Aastedet marked this pull request as ready for review February 7, 2025 13:23
Aastedet
Aastedet commented Feb 7, 2025
View reviewed changes
R/include-diabetes-diagnoses.R
lpr3_diabetes <- join_lpr3(kontakter = kontakter, diagnoser = lpr3_diabetes_diagnoses)

# Define variables
hovedspeciale_other_medical_pattern <- '[Mm]edicin|[Gg]eriatri|[Hh]epatologi|[Hh]æmatologi|[Ii]nfektion|[Kk]ardiologi|[Nn]efrologi|[Rr]eumatologi|[Dd]ermato|[Nn]eurologi|[Oo]nkologi|[Oo]ftalmologi|[Nn]eurofysiologi'
Copy link
Collaborator Author

@Aastedet Aastedet Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is the regex I mentioned.

Maybe it should be moved to algorithm.csv now that lots of logic has been added there (including the string matching logic which the regex is intended to replace) as part of #190

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking that it makes sense to move this to algorithm.csv and maybe adding the potentially capitalised letters in #190 👍

signekb
signekb requested changes Feb 10, 2025
View reviewed changes
Copy link
Contributor

@signekb signekb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice work 🎉
Just a mini review with some initial comments :)

R/include-diabetes-diagnoses.R
verify_required_variables(register_data$diagnoser, "diagnoser")
verify_required_variables(register_data$kontakter, "kontakter")

# Filter and join LPR2 data:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Filter and join LPR2 data:
# Filter and join LPR2 data

R/include-diabetes-diagnoses.R
# To convert the string into an R expression.
rlang::parse_expr()

# Filter to diabetes diagnoses using expression filter.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Filter to diabetes diagnoses using expression filter.
# Filter to diabetes diagnoses using expression filter

R/include-diabetes-diagnoses.R Show resolved Hide resolved
R/include-diabetes-diagnoses.R
lpr3_diabetes <- join_lpr3(kontakter = kontakter, diagnoser = lpr3_diabetes_diagnoses)

# Define variables
hovedspeciale_other_medical_pattern <- '[Mm]edicin|[Gg]eriatri|[Hh]epatologi|[Hh]æmatologi|[Ii]nfektion|[Kk]ardiologi|[Nn]efrologi|[Rr]eumatologi|[Dd]ermato|[Nn]eurologi|[Oo]nkologi|[Oo]ftalmologi|[Nn]eurofysiologi'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking that it makes sense to move this to algorithm.csv and maybe adding the potentially capitalised letters in #190 👍

R/include-diabetes-diagnoses.R
Comment on lines +91 to +110
n_t1d_endocrinology = sum(
(is_t1d == TRUE) &
(is_primary == TRUE) & (department == 'endocrinology'),
na.rm = TRUE
),
n_t2d_endocrinology = sum(
(is_t2d == TRUE) &
(is_primary == TRUE) & (department == 'endocrinology'),
na.rm = TRUE
),
n_t1d_medical = sum(
(is_t1d == TRUE) &
(is_primary == TRUE) & (department == 'other medical'),
na.rm = TRUE
),
n_t2d_medical = sum(
(is_t2d == TRUE) &
(is_primary == TRUE) & (department == 'other medical'),
na.rm = TRUE
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could/should this be moved to some simple helper/filter functions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, yes... something like get_n_type_specific_diagnoses()

It could also be made more succinct by removing all the " == TRUE" parts, since they're not needed. I originally didn't write them out, but I ended up adding the for readability (but now I think they degrade it).

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 Awaiting requested review from lwjohnst86 lwjohnst86 is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@Aastedet Aastedet

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

Successfully merging this pull request may close these issues.

Create function include_diabetes_diagnoses()
2 participants
@Aastedet @signekb