General Issue: Feedback on the release candidate

[bundfussr]

Background Information

• If units are converted in derive_param_waisthgt(), the message is labelled as "ALERT":

   #> ℹ ALERT: Unit conversion performed for "HEIGHT". Values converted from "cm" to
   #> "in".
  

  I would remove it as the meaning is not clear. In STREAM "ALERT" has the same severity as a warning. "ALERT" looks like it should be avoided but the conversion is a feature of the function.
• I would provide an adsl_metabolic dataset instead of dm_metabolic. Then the code in the "Read in Data" section of the vignettes could be simplified.
• I would remove

  # Removing BMI collected at source from the dataset
  advs <- advs %>% filter(!VSTESTCD == "BMI")
  

  from the "Creating a Metabolic ADVS ADaM" vignette because it violates the data minimisation guidance (don't collect data which is not required for the analysis). Furthermore, the code is error-prone. If another parameter was derived before BMI, it would be removed as VSTESTCD is NA for this parameter.
• I think the first paragraph of the "Derive Categorization Variables (AVALCATx, BASECATx)" should be removed because it contradicts the second one.

  admiral does not currently have a generic function to aid in assigning AVALCATy/ AVALCAvN and BASECATy/ BASECAvN values. Below is a simple example of how these values may be assigned:

  For deriving categorization variables (AVALCATx, BASECATx) admiral provides derive_vars_cat() (see documentation of the function for details).

• AVALCATy and BASECATy should be used instead of AVALCATx and BASECATx to be in line with CDISC terminology.
• I would use derive_var_base() (or derive_vars_merged()) for deriving BASECATy and BASECAyN. This avoids defining the categories twice.
• Why are the admiralmetabolic specific functions (derive_param_waisthgt() and derive_param_waisthip()) not used in the "Creating a Metabolic ADVS ADaM" vignette?
• BASECAT1 and BASECA1N are derived in the ADVS vignette but not in the template. Is this intended?
• There are multiple empty lines in a row in the templates. According to the tidyverse style guide this should be avoided.
• It should be CRITyFN instead of CRITyFLN.

Definition of Done

No response

[Fanny-Gautier]

Agree with Stefan's comment. I had to read this section twice, and I guess I got the wrong understanding: thought that there is no specific function to derive AVALCATy directly but can use derive_vars_cat() instead. Remove the first paragraph.

• I think the first paragraph of the "Derive Categorization Variables (AVALCATx, BASECATx)" should be removed because it contradicts the second one.

  admiral does not currently have a generic function to aid in assigning AVALCATy/ AVALCAvN and BASECATy/ BASECAvN values. Below is a simple example of how these values may be assigned:
  For deriving categorization variables (AVALCATx, BASECATx) admiral provides derive_vars_cat() (see documentation of the function for details).

[manciniedoardo]

• If units are converted in derive_param_waisthgt(), the message is labelled as "ALERT":

   #> ℹ ALERT: Unit conversion performed for "HEIGHT". Values converted from "cm" to
   #> "in".
  

I would remove it as the meaning is not clear. In STREAM "ALERT" has the same severity as a warning. "ALERT" looks like it should be avoided but the conversion is a feature of the function.

Issue #69 created.

• I would provide an adsl_metabolic dataset instead of dm_metabolic. Then the code in the "Read in Data" section of the vignettes could be simplified.

Issue #73 created to discuss this.

• I would remove
  # Removing BMI collected at source from the dataset advs <- advs %>% filter(!VSTESTCD == "BMI")
  from the "Creating a Metabolic ADVS ADaM" vignette because it violates the data minimisation guidance (don't collect data which is not required for the analysis). Furthermore, the code is error-prone. If another parameter was derived before BMI, it would be removed as VSTESTCD is NA for this parameter.
• I think the first paragraph of the "Derive Categorization Variables (AVALCATx, BASECATx)" should be removed because it contradicts the second one.
  > admiral does not currently have a generic function to aid in assigning AVALCATy/ AVALCAvN and BASECATy/ BASECAvN values. Below is a simple example of how these values may be assigned:
  >
  > For deriving categorization variables (AVALCATx, BASECATx) admiral provides derive_vars_cat() (see documentation of the function for details).
• AVALCATy and BASECATy should be used instead of AVALCATx and BASECATx to be in line with CDISC terminology.
• I would use derive_var_base() (or derive_vars_merged()) for deriving BASECATy and BASECAyN. This avoids defining the categories twice.
• BASECAT1 and BASECA1N are derived in the ADVS vignette but not in the template. Is this intended?
• There are multiple empty lines in a row in the templates. According to the tidyverse style guide this should be avoided.
• It should be CRITyFN instead of CRITyFLN.

Issue #67 #68 created for all these updates.

• Why are the admiralmetabolic specific functions (derive_param_waisthgt() and derive_param_waisthip()) not used in the "Creating a Metabolic ADVS ADaM" vignette?

This was an oversight - issue #72 created.

[manciniedoardo]

Thanks @bundfussr for the review!