General Issue: Feedback on admiralmetabolic

[rossfarrugia]

Background Information

I mainly focused review around general documentation as I don't really have any experience of this TA. Here's my comments:

• DESCRIPTION file only includes 2 contributors - add others

• NEWS file mentions ADVS template but not ADCOEQ

• Get Started site page has an error:
  

• qs_metabolic - due to the license agreement with University of Leeds i assume we'll always leave this test data only in this package rather than moving to pharmaversesdtm later right? might make things simpler

• ADVS template - I would include in the label line a mention of "for Metabolic Trials" or something to that effect so that it clearly distinguishes from the admiral core ADVS template

• ADVS template and vignette - "Body Mass Index(kg/m2)" should have a space before unit: "Body Mass Index (kg/m2)"

• ADVS template and vignette - constant_by_vars = exprs(USUBJID) - we should try to avoid hardcoding USUBJID - instead more robust to use get_admiral_option("subject_keys")

• ADVS template and vignette - description = "Achievement of ≥ ... - from experience I would avoid using special characters as they only end up causing issues dowsntream depending on different system encodings used, so I would use >= here to keep simple

• ADVS template and vignette - I think someone else mentioned this but you should include waist to height/hip ratio parameters here as you have new functions especially for these

• ADVS template - missing ASEQ derivation

• ADCOEQ template - i don't think library(tibble) is needed for this one

• New functions: why is the constant_by_vars argument only available in one function and not the other? Also as the derivations are so similar it might be worth considering combining these into 1 function? We had similar discussions for {admiralpeds} and ended up keeping 2 functions though as there was enough differences to justify, so completely up to you

• Run the following search in github search bar repo:pharmaverse/admiralmetabolic /[^\x00-\x7F]+/ and it helps you find non-ASCII characters that might cause issues later such as with CRAN - it's mainly cases of ≥ as mentioned above and also ’ in README.

Definition of Done

No response

[manciniedoardo]

• DESCRIPTION file only includes 2 contributors - add others

This was being addressed in #37 which is now merged.

• NEWS file mentions ADVS template but not ADCOEQ

This is being addressed in #61 but it's not merged yet.

• Get Started site page has an error:
  

Issue #66 created.

• qs_metabolic - due to the license agreement with University of Leeds i assume we'll always leave this test data only in this package rather than moving to pharmaversesdtm later right? might make things simpler

Not sure about this. In my mind I didn;t think it a big deal if we moved it later (and asked for permission of course) but I see your point about simplicity too. @AndersAskeland @kathrinflunkert what do you think?

• ADVS template - I would include in the label line a mention of "for Metabolic Trials" or something to that effect so that it clearly distinguishes from the admiral core ADVS template
• ADVS template and vignette - "Body Mass Index(kg/m2)" should have a space before unit: "Body Mass Index (kg/m2)"
• ADVS template and vignette - constant_by_vars = exprs(USUBJID) - we should try to avoid hardcoding USUBJID - instead more robust > to use get_admiral_option("subject_keys")
• ADVS template and vignette - I think someone else mentioned this but you should include waist to height/hip ratio parameters here as you have new functions especially for these
• ADVS template - missing ASEQ derivation

Issues #67, #72 created here. Not including examplws of the ratio functions was an oversight.

• ADCOEQ template - i don't think library(tibble) is needed for this one

issue #68 created.

• New functions: why is the constant_by_vars argument only available in one function and not the other? Also as the derivations are so similar it might be worth considering combining these into 1 function? We had similar discussions for {admiralpeds} and ended up keeping 2 functions though as there was enough differences to justify, so completely up to you

We had extensive discussion around this and settled on two functions. Maybe @yurovska can speak to the constant_by_vars difference as I have memory of this being discussed also?

• ADVS template and vignette - description = "Achievement of ≥ ... - from experience I would avoid using special characters as they only end up causing issues dowsntream depending on different system encodings used, so I would use >= here to keep simple
• Run the following search in github search bar repo:pharmaverse/admiralmetabolic /[^\x00-\x7F]+/ and it helps you find non-ASCII characters that might cause issues later such as with CRAN - it's mainly cases of ≥ as mentioned above and also ’ in README.

Issue #74 created.

[yurovska]

• New functions: why is the constant_by_vars argument only available in one function and not the other? Also as the derivations are so similar it might be worth considering combining these into 1 function? We had similar discussions for {admiralpeds} and ended up keeping 2 functions though as there was enough differences to justify, so completely up to you

We had extensive discussion around this and settled on two functions. Maybe @yurovska can speak to the constant_by_vars difference as I have memory of this being discussed also?

@rossfarrugia @manciniedoardo The reason is that one function involves HEIGHT as an input parameter that can be either measured only once at Screening or by visit (e.g. in pediatric studies), however another one is based on measurements that certainly change over time, so none of the two can be constant.

[rossfarrugia]

thanks both - appreciate all the replies and planned issues for my comments!

[manciniedoardo]

Thanks @rossfarrugia for the review!