Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Closes #34 function for growth parameters for height/length #45
Closes #34 function for growth parameters for height/length #45
Changes from 13 commits
5ba5655
70271b3
e7f97e7
879fe9f
e5a231b
357c726
719fd83
7e3af6e
af7d60c
52b2885
d56dab4
5d11c2f
9bc420f
feba775
0918f3d
c9015a8
d550a0d
14270f6
4fa8ceb
6fd61fc
617f781
b3bf788
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Missing parameters from the issue:
age,
age_unit,
height_age,
These are all needed as we need to know at which age to assume height instead of body length but they're optional as: If only ever length or height is used then leave this NULL and just feed in only the corresponding by length or height metadata (instead of the combined version which has both)
I agree measure argument in the issue not needed - as we can add the height temp var to the input dataset
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.
This is so that you use the right metadata from WHO depending on patient age and which way they were likely measuring (height or body length). it'd be good in the examples to use height_age = 730.5 days i.e. 2 years (even as default from what David explained to us?). See https://github.com/pharmaverse/admiralpeds/blob/35_advs_vignette/vignettes/advs.Rmd from line 229 for further explanation.
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.
@rossfarrugia I think that's why in the example I actually split the dataset into over 2 and under 2 and just ran the function twice, otherwise you would need to create some sort of additional joining variable on both sides, dataset and metadata, which involves additional pre-processing to both datasets, while adding complexity to the function too, the "modularity" of running it twice felt more intuitive to me
I'm open to this adoption with additional arguments, but I wonder what other programmers would think
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.
@zdz2101 i'm open to your approach - it does give the user complete control still. we'll just need to well comment this to explain our approach in your roxygen2 function documentation example (e.g. at the end you should add a comment to explain that the 2 resulting dataframes would need to be set back together to get the complete ADVS for this parameter) and also we'll need to explain well in our template comments and our vignette.
@Fanny-Gautier @Lina2689 what do you think? as the template authors i would trust your advice here as you'll have a good read on what makes this least complex for users.
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.
In the template we selected the records from the metadata where MEASURE ="LENGTH" for patients <730.5 days, and MEASURE="HEIGHT" for patients >=730.5 days. As Zelos mentioned, it will require additional variables to merge the right data depending on the age.
We also added message in the ADVS peds template for the same
message("To derive height/length parameters, below function needs to call separately for Height and Length based on the input data and current age of the patient, as it depends on your CRF guidelines.")
.I think it is easier to split it because if the user has only HEIGHT then there is only one call, similarly for LENGTH. But if the user has both LENGTH and HEIGHT in the data, it will complexify the merge. I think the user has more flexibility while splitting the derivation.
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.
makes sense Fanny, sounds like we have a plan then! and thanks for the other comments here too - looks like me and you picked up similar spots.
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.
Shall we list out the names of the metadata and clarify that "who_wt_for_lgth_boys" and "who_wt_for_lgth_girls" are for subjects with age <730.5 days, and "who_wt_for_ht_boys" and "who_wt_for_ht_girls" are for those with age>=730.5 days? By doing this, the user knows the metadata he/she can use, and also knows to apply different metadata based on subjects' age.
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.
This is a good idea, but I think we'll need to draw up a bigger vignette/article for metadata creation/preprocessing anyway
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.
hopefully the template and vignette can cover this more so user gets the full context
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.
The original WHO metadata table has height/length increment of 0.5, but the metadata in the admiralpeds packages has increment of 0.1. How was it extrapolated? Do we have any documentation on that?
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.
https://www.who.int/tools/child-growth-standards/standards/weight-for-length-height
scroll to the bottom on the
expanded tables
section you will find the increments were by 0.1There 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.
Shall we include a reminder that the expected unit of weight is kg?
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.
can we add comments to this example please? as its quite a lot for users to follow with so much pre-processing before we even get to the example function call
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.
trunc_out = FALSE
can be removed as its default anyway for this function - to simplify the exampleThere 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.
In the ad_advs.R template (PR #32 ), the unit of age is days (with 730.5 days as cutoff). Shall we be consistent in the example 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.
Yes I will fix this
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.
Where is patient 01-701-1033 gone ? While running this code I don't have any records created for patient 01-701-1033. Could you please clarify the reason ? thanks
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.
patient should be in there, at least showing on my end
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.
@zdz2101 please modify the PARAMCD to WGTHSDS/WGTHPCTL to be consistent with the ADVS template. Same changes are required for
derive_params_by_age()
function as well.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.
Shall we include codes to bind rows of
advs_under2
andadvs_over2
?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.
Makes sense just for the sake of completing the example