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

Documentation: unable to derive a new variable using derive_param_computed() when source variable is NA #2338

Closed
w316liu opened this issue Feb 16, 2024 · 21 comments · Fixed by #2428
Assignees
Labels
documentation Improvements or additions to documentation programming

Comments

@w316liu
Copy link
Contributor

w316liu commented Feb 16, 2024

What happened?

I'd like use use derive_param_computed() to create a new parameter for a BDS ADAM (e.g. ADVS). I also wanna directly copy over some source variables to the new parameter, such as ADT, ADY, ADTF, ATMF etc. When the source variables are non-missing, this step is working fine. However, when the source variables are NA for all, the new parameter could not be created properly.

Session Information

R version 4.3.1 (2023-06-16)
admiral_0.12.2

Reproducible Example

# Create a dummy dataset for testing
advs <- tribble(
  ~USUBJID,      ~PARAMCD, ~PARAM,                            ~AVAL, ~AVALU, ~VISIT,
  "01-701-1015", "DIABP",  "Diastolic Blood Pressure (mmHg)",    51, "mmHg", "BASELINE",
  "01-701-1015", "DIABP",  "Diastolic Blood Pressure (mmHg)",    50, "mmHg", "WEEK 2",
  "01-701-1015", "SYSBP",  "Systolic Blood Pressure (mmHg)",    121, "mmHg", "BASELINE",
  "01-701-1015", "SYSBP",  "Systolic Blood Pressure (mmHg)",    121, "mmHg", "WEEK 2",
  "01-701-1028", "DIABP",  "Diastolic Blood Pressure (mmHg)",    79, "mmHg", "BASELINE",
  "01-701-1028", "DIABP",  "Diastolic Blood Pressure (mmHg)",    80, "mmHg", "WEEK 2",
  "01-701-1028", "SYSBP",  "Systolic Blood Pressure (mmHg)",    130, "mmHg", "BASELINE",
  "01-701-1028", "SYSBP",  "Systolic Blood Pressure (mmHg)",    132, "mmHg", "WEEK 2"
)

# Derive a variables with all NAs
advs$ADTF <- NA_character_

# Examples to deriving a new parameter with all source vairables non-missing
# You can see it derives the new parameter "MAP" properly
derive_param_computed(
  advs,
  by_vars = exprs(USUBJID, VISIT),
  parameters = c("SYSBP", "DIABP"),
  set_values_to = exprs(
    AVAL = (AVAL.SYSBP + 2 * AVAL.DIABP) / 3,
    PARAMCD = "MAP",
    PARAM = "Mean Arterial Pressure (mmHg)",
    AVALU = "mmHg"
  )
)

# Examples to deriving a new parameter with one all-NA variable ADTF
# You can see it didn't derive the new parameter "MAP"
derive_param_computed(
  advs,
  by_vars = exprs(USUBJID, VISIT),
  parameters = c("SYSBP", "DIABP"),
  set_values_to = exprs(
    AVAL = (AVAL.SYSBP + 2 * AVAL.DIABP) / 3,
    PARAMCD = "MAP",
    PARAM = "Mean Arterial Pressure (mmHg)",
    AVALU = "mmHg",
    ADTF = ADTF.SYSBP
  )
)
@w316liu w316liu added bug Something isn't working programming labels Feb 16, 2024
@bms63
Copy link
Collaborator

bms63 commented Feb 16, 2024

Hi @w316liu. Many thanks for the bug report!!

I was testing this using admiral 1.0.1 and cannot re-create this bug. Are you able to upgrade to latest admiral?

@w316liu
Copy link
Contributor Author

w316liu commented Feb 16, 2024

Hi @bms63 , I upgraded to admiral 1.0.1. Here is the new R session info.
image

below is the full result of re-running the sample code.

> > # Create a dummy dataset for testing
> advs <- tribble(
+   ~USUBJID,      ~PARAMCD, ~PARAM,                            ~AVAL, ~AVALU, ~VISIT,
+   " ..." ... [TRUNCATED] 

> # Derive a variables with all NAs
> advs$ADTF <- NA_character_

> print(advs)
# A tibble: 8 × 7
  USUBJID     PARAMCD PARAM                            AVAL AVALU VISIT    ADTF 
  <chr>       <chr>   <chr>                           <dbl> <chr> <chr>    <chr>
1 01-701-1015 DIABP   Diastolic Blood Pressure (mmHg)    51 mmHg  BASELINE NA   
2 01-701-1015 DIABP   Diastolic Blood Pressure (mmHg)    50 mmHg  WEEK 2   NA   
3 01-701-1015 SYSBP   Systolic Blood Pressure (mmHg)    121 mmHg  BASELINE NA   
4 01-701-1015 SYSBP   Systolic Blood Pressure (mmHg)    121 mmHg  WEEK 2   NA   
5 01-701-1028 DIABP   Diastolic Blood Pressure (mmHg)    79 mmHg  BASELINE NA   
6 01-701-1028 DIABP   Diastolic Blood Pressure (mmHg)    80 mmHg  WEEK 2   NA   
7 01-701-1028 SYSBP   Systolic Blood Pressure (mmHg)    130 mmHg  BASELINE NA   
8 01-701-1028 SYSBP   Systolic Blood Pressure (mmHg)    132 mmHg  WEEK 2   NA   

> # Examples to deriving a new parameter with all source vairables non-missing
> # You can see it derives the new parameter "MAP" properly
> derive_pa .... [TRUNCATED] 
# A tibble: 12 × 7
   USUBJID     PARAMCD PARAM                            AVAL AVALU VISIT    ADTF 
   <chr>       <chr>   <chr>                           <dbl> <chr> <chr>    <chr>
 1 01-701-1015 DIABP   Diastolic Blood Pressure (mmHg)  51   mmHg  BASELINE NA   
 2 01-701-1015 DIABP   Diastolic Blood Pressure (mmHg)  50   mmHg  WEEK 2   NA   
 3 01-701-1015 SYSBP   Systolic Blood Pressure (mmHg)  121   mmHg  BASELINE NA   
 4 01-701-1015 SYSBP   Systolic Blood Pressure (mmHg)  121   mmHg  WEEK 2   NA   
 5 01-701-1028 DIABP   Diastolic Blood Pressure (mmHg)  79   mmHg  BASELINE NA   
 6 01-701-1028 DIABP   Diastolic Blood Pressure (mmHg)  80   mmHg  WEEK 2   NA   
 7 01-701-1028 SYSBP   Systolic Blood Pressure (mmHg)  130   mmHg  BASELINE NA   
 8 01-701-1028 SYSBP   Systolic Blood Pressure (mmHg)  132   mmHg  WEEK 2   NA   
 9 01-701-1015 MAP     Mean Arterial Pressure (mmHg)    74.3 mmHg  BASELINE NA   
10 01-701-1015 MAP     Mean Arterial Pressure (mmHg)    73.7 mmHg  WEEK 2   NA   
11 01-701-1028 MAP     Mean Arterial Pressure (mmHg)    96   mmHg  BASELINE NA   
12 01-701-1028 MAP     Mean Arterial Pressure (mmHg)    97.3 mmHg  WEEK 2   NA   

> # Examples to deriving a new parameter with one all-NA variable ADTF
> # You can see it didn't derive the new parameter "MAP"
> derive_param_compute .... [TRUNCATED] 
# A tibble: 8 × 7
  USUBJID     PARAMCD PARAM                            AVAL AVALU VISIT    ADTF 
  <chr>       <chr>   <chr>                           <dbl> <chr> <chr>    <chr>
1 01-701-1015 DIABP   Diastolic Blood Pressure (mmHg)    51 mmHg  BASELINE NA   
2 01-701-1015 DIABP   Diastolic Blood Pressure (mmHg)    50 mmHg  WEEK 2   NA   
3 01-701-1015 SYSBP   Systolic Blood Pressure (mmHg)    121 mmHg  BASELINE NA   
4 01-701-1015 SYSBP   Systolic Blood Pressure (mmHg)    121 mmHg  WEEK 2   NA   
5 01-701-1028 DIABP   Diastolic Blood Pressure (mmHg)    79 mmHg  BASELINE NA   
6 01-701-1028 DIABP   Diastolic Blood Pressure (mmHg)    80 mmHg  WEEK 2   NA   
7 01-701-1028 SYSBP   Systolic Blood Pressure (mmHg)    130 mmHg  BASELINE NA   
8 01-701-1028 SYSBP   Systolic Blood Pressure (mmHg)    132 mmHg  WEEK 2   NA   

@bundfussr
Copy link
Collaborator

Hi @w316liu , for such use cases the keep_nas argument is provided. If you set it to TRUE, records are added even if some of the contributing variables are NA. See also the last example in the documentation.

@bms63
Copy link
Collaborator

bms63 commented Feb 19, 2024

Thanks @bundfussr !!1

Wondering if we should make this more prominent for users? The documentation is already quite extensive and might get lost in it...but what do you think?

@w316liu is there some way we could of made this more prominent for you?

@bundfussr
Copy link
Collaborator

Wondering if we should make this more prominent for users? The documentation is already quite extensive and might get lost in it...but what do you think?

It would be helpful if the "Examples" section would be more structured, e.g., a table of contents at the beginning and a title and description for each example. For the SAS tool we had this:
image
However, it is not easy to realize it in R. Adam looked into it some time ago. I think at the moment we do not have resources for such a task.

@bms63 bms63 added documentation Improvements or additions to documentation and removed bug Something isn't working labels Feb 20, 2024
@bms63 bms63 changed the title Bug: unable to derive a new variable using derive_param_computed() when source variable is NA Documentation: unable to derive a new variable using derive_param_computed() when source variable is NA Feb 20, 2024
@bms63
Copy link
Collaborator

bms63 commented Feb 20, 2024

Looking at gt package is interesting for possible solution. looks a little manual, but could add some value

https://gt.rstudio.com/reference/sub_small_vals.html
https://github.com/rstudio/gt/blob/HEAD/R/substitution.R

@bundfussr
Copy link
Collaborator

Looking at gt package is interesting for possible solution. looks a little manual, but could add some value

https://gt.rstudio.com/reference/sub_small_vals.html https://github.com/rstudio/gt/blob/HEAD/R/substitution.R

@bms63 , the links are interesting but I don't understand how they could solve this issue.

@bms63
Copy link
Collaborator

bms63 commented Feb 20, 2024

I was thinking the way gt does it could help break up the wall of code and comments. right now when I look at the examples it is just a huge drop of information. Perhaps we could break up the examples with images that have Example 1, Example 2, etc...this way they could be standardized and used across other examples

@bundfussr
Copy link
Collaborator

I was thinking the way gt does it could help break up the wall of code and comments. right now when I look at the examples it is just a huge drop of information. Perhaps we could break up the examples with images that have Example 1, Example 2, etc...this way they could be standardized and used across other examples

I see. Good idea! We could put each example into a separate subsection. Maybe it is added to the TOC then.

@w316liu
Copy link
Contributor Author

w316liu commented Feb 20, 2024

Thank you @bundfussr for your help. Turning on keep_nas solved my problem. Adding an example is a good idea. Thank you both.

@bms63
Copy link
Collaborator

bms63 commented Feb 20, 2024

@pharmaverse/admiral @pharmaverse/admiral_comm Hi all - Please see above discussion. Anyone interested in exploring possibilities on how to present examples in documentation? This would be a separate issue from this one.

@sophie-gem
Copy link
Collaborator

@pharmaverse/admiral @pharmaverse/admiral_comm Hi all - Please see above discussion. Anyone interested in exploring possibilities on how to present examples in documentation? This would be a separate issue from this one.

@bms63 has anyone picked this up? If not, I'd be happy to take a look at improving the presentation of examples in our function references.

@bms63
Copy link
Collaborator

bms63 commented Mar 20, 2024

That would be amazing!! However, I think this is two separate issues.

The first is about improving the documentation and examples for the NA issues for derive_param_computed.

The second is about improving the overall examples for all admiral functions. I think a separate discussion thread is in order - https://github.com/pharmaverse/admiral/discussions - do you mind making one?

@sophie-gem
Copy link
Collaborator

Apologies if I've misunderstood - but would the first fall out of (the second) an improvement to the general structure of examples? Or do we have an obvious omission in terms of showing what happens with NAs?

@bms63
Copy link
Collaborator

bms63 commented Mar 21, 2024

I think we should aim to improve this function's documentation as best as we can with our current toolset. The new example discussion might take a while to implement!!

@sophie-gem
Copy link
Collaborator

I think we should aim to improve this function's documentation as best as we can with our current toolset. The new example discussion might take a while to implement!!

Aha, yes ok - that makes sense. Thanks for the clarification!

@bms63
Copy link
Collaborator

bms63 commented Apr 23, 2024

HI Sophie - did this one ever get figured ou? I know we moved the bigger topic out to a discussion, but I still think we should tweak this specific documentation

@sophie-gem
Copy link
Collaborator

Hi Ben, so sorry I haven't got around to this yet - my bandwidth is somewhat full currently...if there is someone else who has the time to pick this up - please feel free to reassign. Otherwise, I'll pick it up as soon as I get some capacity.

@bms63
Copy link
Collaborator

bms63 commented Apr 24, 2024

@pharmaverse/admiral @pharmaverse/admiral_comm anyone have some bandwidth to make an additional example for the above situation? The larger discussion on example presentation can be ignored as that is in a different thread now #2387

think this would take an hour @w316liu already gave a potential example!

@manciniedoardo
Copy link
Collaborator

Happy to take this on.

@manciniedoardo manciniedoardo self-assigned this May 8, 2024
@bms63
Copy link
Collaborator

bms63 commented May 8, 2024

Thanks @manciniedoardo !!

manciniedoardo added a commit that referenced this issue May 13, 2024
bms63 added a commit that referenced this issue May 13, 2024
bms63 added a commit that referenced this issue May 13, 2024
…mentation (#2428)

* #2338 added new example in derive_param_computed() showcasing keep_nas = TRUE

* #2338 update following review

* #2338 chore: checks

* fix: #2338 link

---------

Co-authored-by: Ben Straub <ben.x.straub@gsk.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation programming
Development

Successfully merging a pull request may close this issue.

5 participants