-
Notifications
You must be signed in to change notification settings - Fork 10
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
RXR-373: consistent handling of missing/unrecognized data in calc_ibw()
/calc_ffm()
/calc_egfr()
.
#19
Conversation
with warnings for unrecognized sexes, where needed
this refactor looks great at a quick glance! I'm so glad you tackled this - it had been bothering me for a while. |
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.
lgtm - the decision for when to stop versus warn makes sense. I had two comments.
R/calc_egfr.R
Outdated
height = height, | ||
scr = scr | ||
), | ||
FALSE |
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.
do we want this to be FALSE or NULL?
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.
Originally we were returning FALSE here so I kept that, but NULL makes sense too. In practice I don't think it should matter too much -- if the egfr method isn't recognized we will have thrown an error long before we get to this line, so I don't think we should ever reach this fallback. Maybe we could remove it altogether.
R/calc_egfr.R
Outdated
k <- ifelse( | ||
preterm & age < 1, | ||
0.33, | ||
ifelse( | ||
age < 1, | ||
0.45, | ||
ifelse( | ||
age > 13 & sex == 'male', | ||
0.7, | ||
0.55 | ||
) | ||
) | ||
) |
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 know this is how it was written previously, but perhaps this is clearer?
k <- ifelse( | |
preterm & age < 1, | |
0.33, | |
ifelse( | |
age < 1, | |
0.45, | |
ifelse( | |
age > 13 & sex == 'male', | |
0.7, | |
0.55 | |
) | |
) | |
) | |
if (age < 1 ) { | |
k <- ifelse(preterm, 0.33, 0.45) | |
} else if (age > 13) { | |
k <- ifelse(sex == 'male', 0.77, 0.55) | |
} else { | |
k <- 0.55 | |
} |
we then also could only error if sex is not in c("male", "female") if age > 13.
In practice we should not reach this line as unrecognized methods error earlier in the function, but good to have as a backup.
NULL
.For
calc_egfr()
andcalc_ffm()
, the functions were relatively long with conditional branches for each method. Rather than add more lines I opted to split the code into separate functions for each method, which seemed a bit cleaner, but does make the diff rather large. I added a number of tests for thecalc_ffm()
methods;calc_egfr()
was already relatively well covered so I just added a test for the new behavior there.