Enable passing named vector of analysis_decimals to summary.gs_design()

[jdblischak]

Closes #388

This is a work-in-progress. I want to confirm the new behavior before I clean up the code, add tests, and update the documentation (and also apply analogous updates to col_vars and col_decimals).

I've updated the behavior of the argument analysis_decimals to summary.gs_design() to accept a named vector. This enables the user to specifically adjust the number of displayed decimals places for specific variables without having to also include the default values for all the other analysis variables. However, I've also maintained the old behavior.

Here is how it currently functions with this PR:

library("gsDesign2")
x <- gs_design_ahr(analysis_time = c(12, 24))

# Maintain previous behavior
x |> summary() |> attr("groups")
## # A tibble: 2 × 2
## Analysis                                                                              .rows
## <chr>                                                                           <list<int>>
## 1 Analysis: 1 Time: 12 N: 707.3 Event: 160.4 AHR: 0.81 Information fraction: 0.42         [2]
## 2 Analysis: 2 Time: 24 N: 848.8 Event: 385.6 AHR: 0.72 Information fraction: 1            [2]


x |>
  summary(analysis_vars = c("time", "n", "event", "ahr", "info_frac"),
          analysis_decimals = c(2, 0, 0, 4, 4)) |>
  attr("groups")
## # A tibble: 2 × 2
## Analysis                                                                              .rows
## <chr>                                                                           <list<int>>
## 1 Analysis: 1 Time: 12 N: 707 Event: 160 AHR: 0.8108 Information fraction: 0.4191         [2]
## 2 Analysis: 2 Time: 24 N: 849 Event: 386 AHR: 0.7152 Information fraction: 1              [2]

x |>
  summary(analysis_vars = c("ahr", "info_frac"),
          analysis_decimals = c(4, 4)) |>
  attr("groups")
## # A tibble: 2 × 2
## Analysis                                                   .rows
## <chr>                                                <list<int>>
## 1 Analysis: 1 AHR: 0.8108 Information fraction: 0.4191         [2]
## 2 Analysis: 2 AHR: 0.7152 Information fraction: 1              [2]

# New behavior
x |>
  summary(analysis_decimals = c(ahr = 4, info_frac = 4)) |>
  attr("groups")
## # A tibble: 2 × 2
## Analysis                                                                                 .rows
## <chr>                                                                               <list<int>
## 1 Analysis: 1 Time: 12 N: 707.3 Event: 160.4 AHR: 0.8108 Information fraction: 0.4191        [2]
## 2 Analysis: 2 Time: 24 N: 848.8 Event: 385.6 AHR: 0.7152 Information fraction: 1             [2]

[jdblischak]

In order to support both the new (named analysis_decimals) and old (same length analysis_vars and analysis_decimals), I couldn't figure out an elegant solution. I ended up using a big if-else statement since then I at least know I've gone through all the possible permutations.

We could simplify this code if we broke backwards compatibility. I don't know how widely these arguments are currently used. But if we want maximum flexibility, I don't think there are ways around if-else statements.

[LittleBeannie]

Hi @jdblischak , thanks for perfecting the summary function. There are lots of work!

I have 2 comments.

1. When I run the example in line 236 (see https://github.com/jdblischak/gsDesign2/blob/summary-gt-decimals/R/summary.R#L236) followed by as_gt(), I got an error message below.
   

2. Can you provide the default analysis_vars = ..., analysis_decimals = ... , col_vars = ..., col_decimals = ...? I guess these default may help users who are interested in changing only part of valuable's digits. For example, the user may only want to change the info_frac valuable for 4 decimals, and keep the rest of the valuables the same decimals as the default.

[jdblischak]

When I run the example in line 236 (see https://github.com/jdblischak/gsDesign2/blob/summary-gt-decimals/R/summary.R#L236) followed by as_gt(), I got an error message below.

I am able to reproduce this error. However, I am also able to reproduce the error when building the package from the main branch. In other words, this bug was already merged. I will investigate further to determine how to fix it.

Can you provide the default analysis_vars = ..., analysis_decimals = ... , col_vars = ..., col_decimals = ...?

That is a longer term goal. The default values depend on the "method", which is part of the object's class but not used in the method dispatch. I want to overhaul the use of classes and S3 methods in this package, but that will take some effort. However, once it is completed, documenting the different default values for the different methods will be straigtforward. Please see #392 (comment) for more discussion/details

[LittleBeannie]

When I run the example in line 236 (see https://github.com/jdblischak/gsDesign2/blob/summary-gt-decimals/R/summary.R#L236) followed by as_gt(), I got an error message below.

I am able to reproduce this error. However, I am also able to reproduce the error when building the package from the main branch. In other words, this bug was already merged. I will investigate further to determine how to fix it.

Can you provide the default analysis_vars = ..., analysis_decimals = ... , col_vars = ..., col_decimals = ...?

That is a longer term goal. The default values depend on the "method", which is part of the object's class but not used in the method dispatch. I want to overhaul the use of classes and S3 methods in this package, but that will take some effort. However, once it is completed, documenting the different default values for the different methods will be straigtforward. Please see #392 (comment) for more discussion/details

Thanks for the explanations. I agree with you on item 2, where we could build another PR in the future. For item 1, will you fix the error in a new PR? If so, would you like me to get this PR merged first?

[jdblischak]

For item 1, will you fix the error in a new PR? If so, would you like me to get this PR merged first?

That will depend on the fix. I went back a whole month ago, and the error was still there, which seems suspicious. Once I figure it out, I'll propose how to handle this PR in relation to the fix.

[LittleBeannie]

For item 1, will you fix the error in a new PR? If so, would you like me to get this PR merged first?

That will depend on the fix. I went back a whole month ago, and the error was still there, which seems suspicious. Once I figure it out, I'll propose how to handle this PR in relation to the fix.

Sounds great, thanks!

[jdblischak]

I figured it out. There is no bug. It's a namespace problem. Both {gsDesign2} and {gsDesign} define the function as_gt(). If {gsDesign} is loaded after {gsDesign2}, its as_gt() masks the existing as_gt() from {gsDesign2}. The solution, if you want to use as_gt() from {gsDesign2}, is to always load it after you load {gsDesign}. The code below reproduces the namespace conflict between the two packages.

library("gsDesign2")
search()[1:2]
## [1] ".GlobalEnv"        "package:gsDesign2"
environment(as_gt)
## <environment: namespace:gsDesign2>
x_ahr <- gs_design_ahr()
x_ahr |> summary() |> as_gt()

library("gsDesign")
## 
## Attaching package: ‘gsDesign’
## 
## The following objects are masked from ‘package:gsDesign2’:
##   
##   as_gt, as_rtf
search()[1:3]
## [1] ".GlobalEnv"        "package:gsDesign"  "package:gsDesign2"
environment(as_gt)
## <environment: namespace:gsDesign>
x_ahr |> summary() |> as_gt()
## Error in UseMethod("as_gt") : 
##   no applicable method for 'as_gt' applied to an object of class "c('non_binding', 'ahr', 'gs_design', 'grouped_df', 'tbl_df', 'tbl', 'data.frame')"

The example code is actually already written in the correct order, so there is nothing to be done. I assume we both got the error because we built the dev version of {gsDesign2} prior to running the example code interactively, thus causing {gsDesign2} to be loaded first. I confirmed I could run the examples through as_gt() when I carefully loaded {gsDesign} first.

gsDesign2/R/summary.R

Lines 158 to 162 in 2cc2c73

	#' @examples
	#' # Design parameters ----
	#' library(gsDesign)
	#' library(gsDesign2)
	#' library(dplyr)

Since this namespace conflict is unrelated to the changes I made in this PR, if you approve, please go ahead and merge.

[LittleBeannie]

I figured it out. There is no bug. It's a namespace problem. Both {gsDesign2} and {gsDesign} define the function as_gt(). If {gsDesign} is loaded after {gsDesign2}, its as_gt() masks the existing as_gt() from {gsDesign2}. The solution, if you want to use as_gt() from {gsDesign2}, is to always load it after you load {gsDesign}. The code below reproduces the namespace conflict between the two packages.

library("gsDesign2")
search()[1:2]
## [1] ".GlobalEnv"        "package:gsDesign2"
environment(as_gt)
## <environment: namespace:gsDesign2>
x_ahr <- gs_design_ahr()
x_ahr |> summary() |> as_gt()

library("gsDesign")
## 
## Attaching package: ‘gsDesign’
## 
## The following objects are masked from ‘package:gsDesign2’:
##   
##   as_gt, as_rtf
search()[1:3]
## [1] ".GlobalEnv"        "package:gsDesign"  "package:gsDesign2"
environment(as_gt)
## <environment: namespace:gsDesign>
x_ahr |> summary() |> as_gt()
## Error in UseMethod("as_gt") : 
##   no applicable method for 'as_gt' applied to an object of class "c('non_binding', 'ahr', 'gs_design', 'grouped_df', 'tbl_df', 'tbl', 'data.frame')"

The example code is actually already written in the correct order, so there is nothing to be done. I assume we both got the error because we built the dev version of {gsDesign2} prior to running the example code interactively, thus causing {gsDesign2} to be loaded first. I confirmed I could run the examples through as_gt() when I carefully loaded {gsDesign} first.

gsDesign2/R/summary.R

Lines 158 to 162 in 2cc2c73

	#' @examples
	#' # Design parameters ----
	#' library(gsDesign)
	#' library(gsDesign2)
	#' library(dplyr)

Since this namespace conflict is unrelated to the changes I made in this PR, if you approve, please go ahead and merge.

Thanks for efficiently figuring it out. Nice lesson to learn!

[LittleBeannie]

Thanks, @jdblischak !