Skip to content

Conversation

@jdblischak
Copy link
Collaborator

This is a follow-up to PR #579. It introduced the following NOTE from R CMD check:

* checking R code for possible problems ... [15s/10s] NOTE
pw_info: no visible binding for global variable ‘t.1’
  (/home/runner/work/gsDesign2/gsDesign2/check/gsDesign2.Rcheck/00_pkg_src/gsDesign2/R/pw_info.R:189)
Undefined global functions or variables:
  t.1

This is caused by the following line, which uses the non-standard evaluation feature of {data.table} to update the table by reference:

ans[, t.1 := NULL]

There are various workarounds to fix this:

  • Add t.1 <- NULL just above the line. This is sufficient to satisfy R CMD check, but clutters the code with another line related to an unimportant intermediate variable
  • Replace ans[, t.1 := NULL] with ans[["t.1"]] <- NULL. I confirmed via data.table::address() that the latter makes a copy of the data table, but for such a small object, this will have a negligible effect on performance

However, I was concerned why t.1 was introduced in the first place. I realized it came from this step, where t is both calculated and used as a grouping variable. Since t is in by, there is by definition only a single value, and thus no need to run min(t).

gsDesign2/R/pw_info.R

Lines 176 to 182 in 5103d9c

# pool strata together for each time period
tbl_event <- tbl_event[, .(
t = min(t),
event = sum(event),
info0 = sum(info0),
info = sum(info)
), by = .(time, stratum, t, hr)]

After I removed t = min(t), the column t.1 was no longer created because tbl_event only had a single column t. The NOTE is removed, and the tests all pass. I also ran the code examples below and confirmed the results were identical before and after my change:

pw_info()
##   time stratum t  hr  n    event     info    info0
## 1   30     All 0 0.9 12 21.24782 5.300180 5.311956
## 2   30     All 3 0.6 96 37.24314 9.027063 9.310786

pw_info(
  enroll_rate = define_enroll_rate(
    duration = 12,
    rate = 20
  ),
  fail_rate = define_fail_rate(
    duration = c(9, Inf),
    fail_rate = log(2) / c(10, 20),
    hr = c(0.72, 0.72),
    dropout_rate = 0.001
  ),
  total_duration = 50,
  ratio = 1
)
##   time stratum t   hr   n    event     info    info0
## 1   50     All 0 0.72 180 98.70948 24.29858 24.67737
## 2   50     All 9 0.72  60 87.45497 21.86273 21.86374

@jdblischak jdblischak self-assigned this Nov 6, 2025
Copy link
Collaborator

@LittleBeannie LittleBeannie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this, @jdblischak! I appreciate you tackling it.
This PR touches an important low-level function, and it took me a while to fully understand the context. Your fix is very helpful.

@LittleBeannie LittleBeannie merged commit 9098bde into Merck:main Nov 11, 2025
7 checks passed
@jdblischak jdblischak deleted the note-global-t.1 branch November 11, 2025 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants