Add harm bound to AHR designs by LittleBeannie · Pull Request #640 · Merck/gsDesign2

LittleBeannie · 2026-06-10T19:44:58Z

To solve issue #618.

@jdblischak: In addition to "Efficacy" and "Futility" bound, I added "Harm" bound, which sequentially leads to some changes in gs_bound_summary(). The changes in gs_bound_summary() is suggested by GPT5.5. Could you please review if these AI-suggested changes looks good to you?

This reverts commit 9b59c45.

The helper-support-as_rtf.R is auto-sourced by testit, so the explicit source() call failed during R CMD check. Also update as_gt and as_rtf snapshots to reflect the new bound ordering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

- Run roxygenize to sync Rd files with code (fixes WARNING) - Remove test-independent-as_rtf.R (the .md snapshot is sufficient) - Replace all() with bare logical vector in test assertions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…alone Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

yihui · 2026-06-11T20:14:10Z

Note on snapshot test changes

The .md snapshot diffs look large but are caused by a single behavioral change: as_gt() now sorts bounds by factor level order (Efficacy, Futility, Harm) via arrange(x2, Analysis, Bound), whereas previously it used arrange(x2, Analysis) which preserved the summary() order (Futility before Efficacy, from desc(bound) alphabetical sorting).

This means every analysis section in both test-independent-as_gt.md and test-independent-as_rtf.md has its Efficacy and Futility rows swapped. The actual numeric values are unchanged.

The other visible change in as_gt.md is lrrrrr → crrrrr (first column alignment changed from left to center).

yihui · 2026-06-11T20:16:07Z

Correction on the alignment change: the lrrrrr → crrrrr (first column left → center) is not from this PR's code. It's from gt 1.3.0 changing its default LaTeX alignment for the first data column in row-grouped tables. The old snapshot was generated with an older gt version. Since CI also installs gt 1.3.0, the updated snapshot is correct.

LittleBeannie · 2026-06-16T13:23:05Z

Thank you @yihui for helping with the testit snapshot issues!

jdblischak · 2026-06-16T15:12:09Z

+#'
+#' # Example 8 ----
+#' # Design with an additional harm bound
+#' \donttest{


Note to self: we use \donttest{} here to avoid long-running example code

xref: #147

jdblischak · 2026-06-16T15:23:13Z

 } 
 \fontsize{12.0pt}{14.0pt}\selectfont
-\begin{tabular*}{\linewidth}{@{\extracolsep{\fill}}lrrrrr}
+\begin{tabular*}{\linewidth}{@{\extracolsep{\fill}}crrrrr}


In the future, I think it would be cleaner if we fixed snapshot test churn in a separate PR

Yes, that's a good idea.

Previously we skipped these tests on CRAN (via skip_on_cran()) so we have been passively accepting upstream {gt} changes, which I find quite annoying. Now these tests run unconditionally, so we have at least one vote in our hands to restrict {gt}---if they make such changes again, they will have to rethink and inform us in advance, otherwise they will break our tests.

jdblischak · 2026-06-16T15:31:03Z

+  if (nrow(row_bound) == 0) return(rep(NA_real_, length(columns)))
+  as.numeric(unlist(row_bound[1, columns], use.names = FALSE))
+}
+


This refactoring looks good

jdblischak · 2026-06-16T15:37:17Z

+  harm_bound <- x |> dplyr::filter(bound == "harm") |> dplyr::pull(z)
+  futility_bound <- x |> dplyr::filter(bound == "lower") |> dplyr::pull(z)
+  (harm_bound <= futility_bound)
+})


Please instruct the AI to write unit tests for every function that it edits

jdblischak · 2026-06-16T15:38:28Z


  # One-sided design should not include Futility column
  if (all(is.na(out[["Futility"]]))) out[["Futility"]] <- NULL
+  if (all(is.na(out[["Harm"]]))) out[["Harm"]] <- NULL


Changes look good when I tested locally, but this needs some unit tests for long-term maintenance. For example, confirm that a one-sided design does not return the column Harm

jdblischak · 2026-06-16T15:44:54Z

+#'   single value of `TRUE` (default) indicates all analyses;
+#'   single value of `FALSE` indicates no harm bound; otherwise,
+#'   a logical vector of the same length as `info` should
+#'   indicate which analyses will have a harm bound.


What is the relationship between test_harm and test_lower? Is it theoretically possible to have a harm bound but not a futility bound?

From my local testing, it appears that a harm bound is only included if both test_harm and test_lower are TRUE for a given analysis. If this is the expected behavior, please document this. If it does not make sense to have a harm bound when there is no futility bound, then we should also consider throwing an error if test_harm is TRUE and test_lower is FALSE.

jdblischak · 2026-06-16T15:59:18Z

+  upper = gs_b, upar = -qnorm(0.025), test_upper = TRUE,
+  lower = gs_b, lpar = -1, test_lower = TRUE,
+  harm = gs_b, hpar = -2, test_harm = TRUE
+)


Does it make sense to compute and return a futility or harm bound when there is only a single analysis?

If I run this with test_harm = FALSE, it only returns an efficacy bound for the single analysis. But with test_harm = TRUE, it returns all 3 bounds (efficacy, futility, harm).

LittleBeannie added 4 commits June 10, 2026 15:41

Add harm bound to gs_power_npe and gs_design_npe

489ed2a

Add harm bound to gs_xxx_ahr

5e415a4

Update summary functions when harm bound is added

568d492

add developer tests

399cd2a

LittleBeannie self-assigned this Jun 10, 2026

LittleBeannie linked an issue Jun 10, 2026 that may be closed by this pull request

Add harm bound #618

Open

LittleBeannie and others added 9 commits June 11, 2026 10:31

Add as_rtf test file and update snapshot

f2bcae3

Merge remote-tracking branch 'origin/main' into 618-add-harm-bound

f585c45

Rename test file to match new naming convention

9b59c45

Revert "Rename test file to match new naming convention"

b577e09

This reverts commit 9b59c45.

Rename testing file

6d1efe7

Clarify test skill: helpers are auto-sourced, .md snapshots are stand…

fa84635

…alone Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Fix parse error in gs_power_ahr examples (missing # before Example 5)

b561720

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

LittleBeannie requested review from jdblischak and keaven June 16, 2026 13:25

jdblischak reviewed Jun 16, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add harm bound to AHR designs#640

Add harm bound to AHR designs#640
LittleBeannie wants to merge 13 commits into
mainfrom
618-add-harm-bound

LittleBeannie commented Jun 10, 2026 •

edited by jdblischak

Loading

Uh oh!

yihui commented Jun 11, 2026

Uh oh!

yihui commented Jun 11, 2026

Uh oh!

LittleBeannie commented Jun 16, 2026

Uh oh!

jdblischak Jun 16, 2026

Uh oh!

jdblischak Jun 16, 2026

Uh oh!

yihui Jun 16, 2026

Uh oh!

jdblischak Jun 16, 2026

Uh oh!

jdblischak Jun 16, 2026

Uh oh!

jdblischak Jun 16, 2026

Uh oh!

jdblischak Jun 16, 2026

Uh oh!

jdblischak Jun 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

LittleBeannie commented Jun 10, 2026 • edited by jdblischak Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yihui commented Jun 11, 2026

Note on snapshot test changes

Uh oh!

yihui commented Jun 11, 2026

Uh oh!

LittleBeannie commented Jun 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LittleBeannie commented Jun 10, 2026 •

edited by jdblischak

Loading