Refactor `as_rtf()` #450

yihui · 2024-08-12T21:03:24Z

Massive refactoring of as_rtf() to share code with as_gt().

… return a single string

…reused by both as_gt() and as_rtf()

…d of hard-coding `"Null hypothesis"` here?

…ly used inside as_rtf(); is this someone's oversight?

…` vector

R/as_rtf.R

…t will break a test), and I think we should do the same in as_gt()

…r in LaTeX)

…le` / `gs_method_subtitle`

…gn()

…pse) in the end

…`footnote_non_binding()`

…tially buggy (when `N >= 10`)

…e, so move them into `gs_design_info()`

…()` can match the number of rows in the data

jdblischak · 2024-08-14T15:15:01Z

xref: #282

R/as_gt.R

yihui · 2024-08-14T15:36:53Z

R/as_gt.R

@@ -242,115 +250,24 @@ as_gt.gs_design <- function(
    display_inf_bound = FALSE,
    ...) {

-  method <- intersect(class(x), c("ahr", "wlr", "combo", "rd"))[1]
-  full_alpha <- attr(x, "full_alpha")


@LittleBeannie I have a question regarding a difference between as_gt() and as_rtf() that I noticed while comparing the two: for as_gt(), we obtain full_alpha from the attribute of x, whereas for as_rtf(), full_alpha is its argument and not retrieved from x's attributes. Is this expected? I'm not sure if you or @elong0527 is the right person to ask.

Hi @yihui , yes, I can answer the question. The full_alpha should be retrieved from x's attributes. I will prepare a PR to get it there.

If you want to do it, please wait until this PR is merged, or I can just do it in this PR.

Thanks for the confirmation. I know there are many changes occurring in this pull request, so let's wait until this PR is merged.

LittleBeannie

Hi @yihui, there are lots of work, thank you so much! I only have some very minor comments, regarding the naming of the utility functions.

LittleBeannie · 2024-08-15T17:08:24Z

R/as_gt.R

+}
+
+# get the fixed design method
+fixed_method <- function(x) {


Is it appropriate to refer to this as the 'fixed_design_method'? From my perspective, the term 'fixed method' implies that the method is fixed, which may have limited clinical significance.

Furthermore, I've observed utility functions for both fixed design (such as fixed_method, fixed_title, method_footnote), as well as group sequential design (such as get_display_columns , footnote_content, footnote_non_binding, footnote_row, gs_design_info). I am considering whether it would be beneficial to use distinct prefixes for these two sets of utilities to differentiate their purposes.

Finally, I am wondering if we should save the above 2 sets of utilities into a separate R file, and name it as utility_as_gt.

I am considering whether it would be beneficial to use distinct prefixes for these two sets of utilities to differentiate their purposes.

I'm using the prefixes fd_ and gsd_ now, and am open to other suggestions. I tend not to use the prefix gs_design_ because I noticed that we have had functions with this prefix but serve a very different purpose. I hope the abbreviations fd and gsd make sense. They are only for ourselves and users won't use them, so the naming is perhaps not critically important. Anyway, naming is really hard :)

Finally, I am wondering if we should save the above 2 sets of utilities into a separate R file, and name it as utility_as_gt.

I don't have an opinion on that. I can certainly put these functions in as_utils.R if you prefer.

I like the fd and gsd, which are sufficiently clear for developers. I am also good for the current folder/file structure. I will get it merged. Thanks for all the work!

LittleBeannie · 2024-08-15T17:10:39Z

R/as_gt.R

-  if (is.null(title)) title <- sprintf("Fixed Design %s Method", switch(
-    design_mtd,
+# get the default title
+fixed_title <- function(method) {


Echo with my previous comment, can we call this function as fixed_design_title?

LittleBeannie · 2024-08-15T17:13:10Z

R/as_gt.R

-  if (is.null(footnote)) footnote <- switch(
-    design_mtd,
+# get the default footnote
+method_footnote <- function(x, method) {


I am wondering why the prefix of fixed_ is missing... Is the method_footnote appliable for both fixed design and group sequential design?

I was simply being lazy. It was only for fixed design.

LittleBeannie · 2024-08-15T17:14:49Z

R/as_gt.R

@@ -242,115 +250,24 @@ as_gt.gs_design <- function(
    display_inf_bound = FALSE,
    ...) {

-  method <- intersect(class(x), c("ahr", "wlr", "combo", "rd"))[1]
-  full_alpha <- attr(x, "full_alpha")


Hi @yihui , yes, I can answer the question. The full_alpha should be retrieved from x's attributes. I will prepare a PR to get it there.

LittleBeannie · 2024-08-15T17:19:26Z

R/as_gt.R

+}
+
+# a list of information for `as_[gt|rtf].gs_design()` methods
+gs_design_info <- function(


I am hesitant to use gs_design_info as the name, as info in gsDesign2 carries a specific meaning – statistical information (1/variance). Can we consider an alternative name for it?

The naming of this function is the hardest. I spent nearly half an hour on it but couldn't think of a better name. I ended up using gsd_parts(). Please feel free to suggestion a different one.

Agree, naming is always the hardest part. As long as we could get rid of the info as keywords, we should be good. The new name gsd_parts looks good to me.

yihui added 6 commits August 12, 2024 15:16

tweak error message

d01df19

remove the redundant paste0() call: paste(collapse = ' | ') will just…

5485aec

… return a single string

introduce the %||% operator

1a0da68

merge the two assignments

fc0d3e1

"weld" the few short pipes into one expression

52a5165

factor out the code to generate the default title and footnote to be …

cb14c1b

…reused by both as_gt() and as_rtf()

yihui force-pushed the as_rtf_tweaks branch from 235466b to cb14c1b Compare August 12, 2024 21:05

yihui added 14 commits August 12, 2024 16:27

an alternative way to find the method

4c92673

perhaps we should use colname_spannersub[2] as in as_gt(), instea…

5e493b5

…d of hard-coding `"Null hypothesis"` here?

copy from as_gt()

6dd6164

copying from as_gt(): the argument display_inf_bound was not actual…

becd0e0

…ly used inside as_rtf(); is this someone's oversight?

rename design_method() to fixed_design_method()

642e498

factor out gs_design_method()

e271130

copy from as_gt() (will see how to refactor the code later)

c2c640e

copy from as_rtf.fixed_design()

26c3d2b

use %||%

6b34090

2:length is equivalent to -1 (well, assuming length >= 2)

98c0bca

simplify these border variables a little bit

680fc01

intToUtf8() sounds like an overkill; we can simply index the `letters…

ee5387a

…` vector

factor out the row/column indices to make [] thinner

709bc05

I don't think footnotes can be NULL

a16e25c

yihui force-pushed the as_rtf_tweaks branch from 033ba7f to a16e25c Compare August 12, 2024 22:56

yihui commented Aug 12, 2024

View reviewed changes

R/as_rtf.R Outdated Show resolved Hide resolved

R/as_rtf.R Outdated Show resolved Hide resolved

yihui added 7 commits August 12, 2024 20:48

this element should be hard-coded as "Null hypothesis" (otherwise i…

f705696

…t will break a test), and I think we should do the same in as_gt()

factor out more strings into footnote_content()

bb3397a

reuse footnote_content() and eliminate line breaks (which don't matte…

b0fb333

…r in LaTeX)

amend c2c640e: factor out the default footnote content

73370df

it may be more readable without using a closure from local()

9c7308a

rename method_title to fixed_method_title, and add `gs_method_tit…

ebfb904

…le` / `gs_method_subtitle`

factor out the code to get/transform display_columns

7899504

yihui added 9 commits August 13, 2024 09:20

cosmetic changes

584f485

factor out the code to add footnotes and write RTF

4e321d2

delete an extra space (nobody has discovered this before? unbelievable)

998ef29

factor out the code to check the col_rel_width

bd00877

factor out more common code from as_gt.gs_design() and as_rtf.gs_desi…

247b00e

…gn()

collect footnotes using c() and concatenate all with paste(..., colla…

476a635

…pse) in the end

move the logic inherits('non_binding') & x_alpha < full_alpha into …

ac4a83d

…`footnote_non_binding()`

factor out substr(Analysis, 1, 11) == 'Analysis: N', which is poten…

49b23a9

…tially buggy (when `N >= 10`)

the functions gs_[sub]title are simple enough and not used elsewher…

9522d70

…e, so move them into `gs_design_info()`

yihui changed the title ~~Tweaking as_rtf()~~ Refactor as_rtf() Aug 13, 2024

yihui added 2 commits August 13, 2024 14:22

use the processed x from info so that the length of `footnote_row…

a598ec6

…()` can match the number of rows in the data

bump version

3654e0f

yihui marked this pull request as ready for review August 13, 2024 19:34

jdblischak requested review from jdblischak and LittleBeannie August 14, 2024 15:10

jdblischak approved these changes Aug 14, 2024

View reviewed changes

R/as_gt.R Show resolved Hide resolved

yihui commented Aug 14, 2024

View reviewed changes

yihui added 2 commits August 14, 2024 14:10

more comments

5397198

misplaced ) [ci skip]

954a226

LittleBeannie reviewed Aug 15, 2024

View reviewed changes

yihui added 2 commits August 16, 2024 09:03

Merge branch 'main' into as_rtf_tweaks

d353d81

renaming using the prefixes fd_ and gsd_

12cec74

LittleBeannie self-requested a review August 22, 2024 17:23

LittleBeannie approved these changes Aug 22, 2024

View reviewed changes

LittleBeannie merged commit b8b3deb into Merck:main Aug 22, 2024
7 checks passed

yihui deleted the as_rtf_tweaks branch August 22, 2024 17:54

yihui mentioned this pull request Aug 22, 2024

Remove the full_alpha argument from as_rtf.gs_design() #458

Merged

LittleBeannie assigned yihui Aug 29, 2024

LittleBeannie mentioned this pull request Aug 29, 2024

Tidy up summary and as_gt functions #282

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `as_rtf()` #450

Refactor `as_rtf()` #450

yihui commented Aug 12, 2024 •

edited

Loading

jdblischak commented Aug 14, 2024

yihui Aug 14, 2024

LittleBeannie Aug 15, 2024

yihui Aug 15, 2024

LittleBeannie Aug 15, 2024

LittleBeannie left a comment

LittleBeannie Aug 15, 2024

yihui Aug 16, 2024

LittleBeannie Aug 16, 2024

LittleBeannie Aug 15, 2024

LittleBeannie Aug 15, 2024

yihui Aug 16, 2024

LittleBeannie Aug 15, 2024

LittleBeannie Aug 15, 2024

yihui Aug 16, 2024

LittleBeannie Aug 16, 2024

Refactor as_rtf() #450

Refactor as_rtf() #450

Conversation

yihui commented Aug 12, 2024 • edited Loading

jdblischak commented Aug 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LittleBeannie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Refactor `as_rtf()` #450

Refactor `as_rtf()` #450

yihui commented Aug 12, 2024 •

edited

Loading