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.

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

Feature Request: derive_param_computed() wrapper for waist/hip or waist/height ratio #31

Closed
manciniedoardo opened this issue Sep 25, 2024 · 21 comments · Fixed by #33
Closed
Assignees
Labels
enhancement New feature or request

Comments

@manciniedoardo
Copy link
Collaborator

manciniedoardo commented Sep 25, 2024

Feature Idea

As suggested by @starosto, waist/hip ratio and waist/height ratio are common calculations done at ADaM level. These could be easily added as ADVS params using admiral::derive_param_computed(). But should we make a wrapper function for it, similar to derive_param_bmi() or derive_param_bsa()

If we decide to implement, should we have one or two functions?

Relevant Input

See derive_param_bsa examples.

Relevant Output

See derive_param_bsa examples.

Reproducible Example/Pseudo Code

NA

@manciniedoardo manciniedoardo added the enhancement New feature or request label Sep 25, 2024
@manciniedoardo
Copy link
Collaborator Author

@pharmaverse/admiralmetabolic thoughts?

@starosto
Copy link

We keep it simple, just 100*WSTCIR /HIPCIR

@kathrinflunkert
Copy link
Contributor

Why multiplied by 100? Shouldn't a ratio be 1 rather than 100 if e.g. Waist and Hip circumference are both 100 cm?
100/100 = 1

@starosto
Copy link

we report this parameter in %

@kathrinflunkert
Copy link
Contributor

Interesting. For me the term 'ratio' implied it is rather a unitless parameter (fraction of 1).
@ Novo team: How is it done on your end?
In this publication (https://www.nature.com/articles/s41591-024-02996-7) it sounds like Novo does it as a fraction of 1:
"Waist-to-height ratio
At baseline, mean WHtR was 0.66 for the study population. The lowest tertile of the SELECT population at baseline had a mean WHtR <0.62, which is higher than the cutoff point of 0.5 used to indicate increased cardiometabolic risk, [...]"

@AndersAskeland
Copy link
Member

AndersAskeland commented Sep 26, 2024

I just looked it up in our (Novo) metadata system, and it has the possibility of being represented as either ratio or percentage. But I would argue that this is a mistake in our system, and the only possible representation should be a ratio. From my understanding, even though ratios can be represented at percentages, it does not make it a percentage.

Imagine for instance a WHR above 1 (which is possible), providing percentages can appear really strange. So, I would vote for us to use the ratio and not provide any functionality for percentages.

@starosto
Copy link

Sorry for the confusion. It should be simple ratio WSTCIR /HIPCIR as Kathrin suggested.

@kathrinflunkert
Copy link
Contributor

No need to apologize. :)
It was good to have this discussion!

@yurovska
Copy link
Contributor

yurovska commented Sep 27, 2024

I would suggest 3 functions:

  1. derive_param_ratio() - a generic wrapper for admiral::derive_param_computed() that derives any ratio parameter in any ADaM BDS (could be useful also for other potential ratio parameters, e.g. Albumin/Creatinine Ratio that is specifically mentioned in TAUG for Diabetes, 3.1.3 Kidney Function, or things like Glucose/Insulin Ratio, etc.);
  2. derive_param_waisthip() - a wrapper for derive_param_ratio() to derive Waist-to-Hip Ratio;
  3. derive_param_waisthgt() - a wrapper for derive_param_ratio() to derive Waist-to-Height Ratio.

+ supplemental compute_ratio() (similar to admiral::compute_bmi(), admiral::compute_bsa(), etc.) that properly handles devision by zero (just in case).

Actually, I already have a draft implementation and would be more than happy to share. Looking forward to contributing to @pharmaverse/admiralmetabolic!

yurovska added a commit to yurovska/admiralmetabolic that referenced this issue Sep 28, 2024
yurovska added a commit to yurovska/admiralmetabolic that referenced this issue Sep 28, 2024
yurovska added a commit to yurovska/admiralmetabolic that referenced this issue Sep 28, 2024
@manciniedoardo
Copy link
Collaborator Author

I would suggest 3 functions:

  1. derive_param_ratio() - a generic wrapper for admiral::derive_param_computed() that derives any ratio parameter in any ADaM BDS (could be useful also for other potential ratio parameters, e.g. Albumin/Creatinine Ratio that is specifically mentioned in TAUG for Diabetes, 3.1.3 Kidney Function, or things like Glucose/Insulin Ratio, etc.);
  2. derive_param_waisthip() - a wrapper for derive_param_ratio() to derive Waist-to-Hip Ratio;
  3. derive_param_waisthgt() - a wrapper for derive_param_ratio() to derive Waist-to-Height Ratio.
  • supplemental compute_ratio() (similar to admiral::compute_bmi(), admiral::compute_bsa(), etc.) that properly handles devision by zero (just in case).

Actually, I already have a draft implementation and would be more than happy to share. Looking forward to contributing to @pharmaverse/admiralmetabolic!

This is a nice idea for setup - I am wondering though if the ratio function would be better in {admiral}? @bms63 @bundfussr what do you think?

@bms63
Copy link

bms63 commented Sep 30, 2024

If it can be used by other TAs or general ADaMS then yes we should move it to admiral

@yurovska
Copy link
Contributor

Good point. It's indeed more suitable for {admiral}.

What would be the workflow in this kind of case? We propose it to {admiral} by creating an issue/PR there and wait till it becomes available? Or as an alternative we could make it internal here for the moment (@export flag to be removed) and create another issue for the future to replace it with the one from admiral when/if available.

@bms63
Copy link

bms63 commented Sep 30, 2024

Yes - create issue and do a PR. Be best if you work on it or someone from your team.

We would merge it to main - and it can easily become available with Remotes or staged.dependencies.

There is an admiral released planned for January 2025 that could include this...but we can do a mini-release of admiral if things don't line up perfectly.

@bundfussr
Copy link
Contributor

I would suggest 3 functions:

  1. derive_param_ratio() - a generic wrapper for admiral::derive_param_computed() that derives any ratio parameter in any ADaM BDS (could be useful also for other potential ratio parameters, e.g. Albumin/Creatinine Ratio that is specifically mentioned in TAUG for Diabetes, 3.1.3 Kidney Function, or things like Glucose/Insulin Ratio, etc.);
  2. derive_param_waisthip() - a wrapper for derive_param_ratio() to derive Waist-to-Hip Ratio;
  3. derive_param_waisthgt() - a wrapper for derive_param_ratio() to derive Waist-to-Height Ratio.
  • supplemental compute_ratio() (similar to admiral::compute_bmi(), admiral::compute_bsa(), etc.) that properly handles devision by zero (just in case).

Actually, I already have a draft implementation and would be more than happy to share. Looking forward to contributing to @pharmaverse/admiralmetabolic!

This is a nice idea for setup - I am wondering though if the ratio function would be better in {admiral}? @bms63 @bundfussr what do you think?

If the ratio function are implemented, it should be in {admiral} because ratios are not metabolic specific.

However, I wouldn't implement this setup. We have something similar in admiral but I would consider it as historic. I see no benefit in implementing these functions. With these function we could for example derive the waist-hip ratio by

derive_param_waisthip(
  advs,
  by_vars = exprs(STUDYID, USUBJID, ADT),
  wstcir_code = "WSTCIR",
  hipcir_code = "HIPCIR",
  set_values_to = exprs(
   PARAMCD = "WAISTHIP"
  )
)

instead of

derive_param_computed(
  advs,
  by_vars = exprs(STUDYID, USUBJID, ADT),
  parameters = c("WSTCIR", "HIPCIR"),
  set_values_to = exprs(
   AVAL = AVAL.WSTCIR / AVAL.HIPCIR,
   PARAMCD = "WAISTHIP"
  )
)

The first call doesn't look simpler, shorter, or clearer to me. I think the second call is even clearer than the first one because it is obvious how the ratio derived. Although the new functions are mainly wrapper functions they need to be implemented, documented, and tested, which is some work. And the users and reviewers would need to learn more functions.

I would suggest not implementing new derive_param_*() functions for parameters which are defined by a formula. For complex formulas like "Framingham Heart Study Cardiovascular Disease 10-Year Risk Score" a compute function should be implemented. I don't think we need a compute function for ratios.

@yurovska
Copy link
Contributor

yurovska commented Oct 1, 2024

Thanks @bundfussr for your feedback.

I don't think the point of creating wrapper functions is only to simplify the call. It's more about specializing them for a certain purpose. It’s easy for you, with your vast experience with {admiral}, to understand which function is best and how exactly to use it for specific purposes, but not for a user who installed the package five minutes ago. If they need parameters for a ratio, the first thing they’ll do is enter specific keywords in the search bar, and quickly find what they need.

In fact, the derive_param_waisthip function does simplify the call a bit. Firstly, it provides default values right away, so you can omit some arguments, which the generic function doesn’t do, and also, as you may have noticed, it lacks the constant_parameters and constant_by_vars arguments, as they don’t make sense specifically for Waist and Hip measurements, and will only be confusing.

Besides, specialized wrapper functions may have additional checks for the input arguments or data in order to make it more fool-proof and/or provide the user with some useful info regarding the correctness of their input data.

It reminded me of my yesterday’s struggle with the admiral::derive_var_joined_exist_flag function, which is so freaking flexible that it took me a couple of hours to make it do something very simple, which would have taken 2-3 lines in a SAS DATA step. I have a strong feeling to vote even for wrappers of the wrappers if it puts the right values to the right arguments for me.

@bms63
Copy link

bms63 commented Oct 4, 2024

Thanks for the feedback @yurovska on admiral

It reminded me of my yesterday’s struggle with the admiral::derive_var_joined_exist_flag function, which is so freaking flexible that it took me a couple of hours to make it do something very simple, which would have taken 2-3 lines in a SAS DATA step. I have a strong feeling to vote even for wrappers of the wrappers if it puts the right values to the right arguments for me.

Is it possible for you to reconstruct this simple example so we can include it in our documentation.

@yurovska
Copy link
Contributor

yurovska commented Oct 4, 2024

Is it possible for you to reconstruct this simple example so we can include it in our documentation.

@bms63 Off-topic here but ok, here it is:

library(tibble)

ex <- tribble(
  ~STUDYID, ~USUBJID, ~EXDOSE, ~EXSTDTM,
  
  "STUDY001", "SUBJ001", 2, "2024-01-01T08:00",
  "STUDY001", "SUBJ001", 4, "2024-01-02T08:00",
  
  "STUDY001", "SUBJ002", 2, "2024-01-01T08:30",
  "STUDY001", "SUBJ002", 4, "2024-01-02T08:30",
  "STUDY001", "SUBJ002", 2, "2024-01-03T08:30",   # Down-titration
  "STUDY001", "SUBJ002", 2, "2024-01-04T08:30",
  
  "STUDY001", "SUBJ003", 2, "2024-01-01T09:00",
  "STUDY001", "SUBJ003", 4, "2024-01-03T09:00",
  "STUDY001", "SUBJ003", 4, "2024-01-04T09:00",
  "STUDY001", "SUBJ003", 8, "2024-01-05T09:00",
  "STUDY001", "SUBJ003", 4, "2024-01-06T09:00",   # Down-titration
  
  "STUDY001", "SUBJ004", 2, "2024-01-01T10:00",
  "STUDY001", "SUBJ004", 4, "2024-01-02T10:00",
  "STUDY001", "SUBJ004", 4, "2024-01-03T10:00"
)

ex_flag <- ex %>%
  # Flag each EXDOSE which is lower than previous per patient
  derive_var_joined_exist_flag(
    dataset_add = ex,
    by_vars = exprs(STUDYID, USUBJID),
    order = exprs(EXSTDTM),
    new_var = DOSE_REDUCED,
    tmp_obs_nr_var = tmp_dose_nr,
    join_vars = exprs(EXDOSE),
    join_type = "before",
    first_cond_lower = NULL,
    first_cond_upper = NULL,
    filter_add = NULL,
    filter_join = (
      tmp_dose_nr == tmp_dose_nr.join + 1   # Look only at adjacent doses (i.e. next to each other)
      & EXDOSE > 0 & EXDOSE.join > 0        # Both doses are valid
      & EXDOSE < EXDOSE.join                # Dose is lower than previous
    )
  )
data ex_flag (drop = prev_dose);
    set ex;
    by USUBJID EXSTDTM;
    
    if first.USUBJID then call missing(prev_dose);
    else prev_dose = lag(EXDOSE);
    
    if 0 < EXDOSE < prev_dose < 0 then DOSE_REDUCED = "Y";
run;

@bms63
Copy link

bms63 commented Oct 4, 2024

Thanks @yurovska - I made it an issue in admiral.

@AndersAskeland
Copy link
Member

I think we should discuss this issue during our meeting today. To summarize, I think we need to consider the following:

  • Will derive_param_ratio() be added to admiral?
  • If derive_param_ratio() is not added, should we have dedicated functions for calculating waist hip ratio and waist height ratio?
  • If derive_param_ratio() is added, does it make sense to create dedicated functions for waist/hip and waist/height or should we just call derive_vars_ratio()?

@manciniedoardo
Copy link
Collaborator Author

manciniedoardo commented Oct 9, 2024

Summary of discussion and decision at standup:

  • The ratio function will not be implemented at the {admiral} level due to the comments by @bms63 and @bundfussr
  • Within {admiralmetabolic}, we will go ahead and implement one or two wrappers for derive_param_computed() to derive waist to hip ratio and waist to height ratio, notwithstanding the fact that these functions will be relatively simple. This is because:
    • a) We don't have functions in the package yet and it would do the team good to see how they are implemented in terms of documentation, reference pages, unit tests, etc.
    • b) The function(s) can also do some unit conversions on the fly to calculate the unitless ratios as the team has indicated that at times the two tests are measured in different units (inches vs cm or similar).
    • c) We can always remove/supersede later on.

To that end, at your earliest convenience @yurovska please update #33 taking note of the above. We leave to your discretion (and the eventual reviewers'):

  • Whether you want to implement two separate functions for waist/hip and waist/height, or one.
  • Whether you still want to use an intermediate ratio function (especially if you implement two functions), as long as it's marked as not exported.

yurovska added a commit to yurovska/admiralmetabolic that referenced this issue Oct 9, 2024
yurovska added a commit to yurovska/admiralmetabolic that referenced this issue Oct 10, 2024
yurovska added a commit to yurovska/admiralmetabolic that referenced this issue Oct 10, 2024
@yurovska
Copy link
Contributor

yurovska commented Oct 10, 2024

derive_param_ratio is now an internal function (not exported).
Both derive_param_waisthip and derive_param_waisthgt are kept.
Units conversion on the fly has been implemented.

yurovska added a commit to yurovska/admiralmetabolic that referenced this issue Oct 11, 2024
yurovska added a commit to yurovska/admiralmetabolic that referenced this issue Oct 15, 2024
yurovska added a commit to yurovska/admiralmetabolic that referenced this issue Oct 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants