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.

Sign up for GitHub

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

Tweaking as_gt() #449

Merged
merged 24 commits into from
Aug 12, 2024
Merged

Tweaking as_gt() #449

Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
c65e170
use is.numeric() instead of is.vector(, 'numeric')
yihui Aug 9, 2024
aaccf68
use intersect() to find the class name to make the code terse
yihui Aug 9, 2024
88d6650
remove curly braces (note that 'rd' appeared twice in switch() in the…
yihui Aug 9, 2024
768edcc
merge the cases for 'mb', 'milestone', and 'rmst' in switch() by putt…
yihui Aug 9, 2024
7e2e2e2
use attr() to retrieve a specific attribute instead of retrieving all…
yihui Aug 9, 2024
caa87e1
find the method by intersect(), similar to 691b5d4aa2908fe021018abd78…
yihui Aug 9, 2024
e08779c
use inherits() instead of testing class() directly
yihui Aug 9, 2024
cb4f64b
substr() and substring() are vectorized, so no need to lapply()
yihui Aug 9, 2024
d39e657
just merge the expressions since there are only two, and probably not…
yihui Aug 9, 2024
fd923a7
factor out `if (is.null(title | subtitle))`, and change `if (method)`…
yihui Aug 9, 2024
9bf78b7
factor out the common elements in `display_columns` and add the only …
yihui Aug 9, 2024
ae7a8ce
the name 'display_columns' can appear 3 times instead of 6
yihui Aug 9, 2024
747cdf3
`sum(!cond) >= 1` is equivalent to `!all(cond)` and the latter is muc…
yihui Aug 9, 2024
e6f357c
store the conditions in `i1` and `i2` so that we don't have to repeat…
yihui Aug 9, 2024
fbc081b
merge the two conditions by `&` in subset()
yihui Aug 9, 2024
812c3f5
find the two names and substitute them via one operation
yihui Aug 9, 2024
7863194
`if (!is.null(x)) if (length(x) != 0) for (i in seq_along(x))` is equ…
yihui Aug 9, 2024
8e4bad5
factor out footnote_non_binding() so it can also be reused in as_rtf()
yihui Aug 9, 2024
17fce05
delete 's' [ci skip]
yihui Aug 9, 2024
2267998
use a string template instead
yihui Aug 9, 2024
24113fd
get rid of optional quotes
yihui Aug 9, 2024
7168d2d
bump version [ci skip]
yihui Aug 9, 2024
90352a0
explicit return
yihui Aug 12, 2024
2494e57
fix title for the 'rd' method
yihui Aug 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions R/ahr_blinded.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,14 +84,14 @@ ahr_blinded <- function(
hr = c(1, .6),
ratio = 1) {
# Input checking
if (!is.vector(hr, mode = "numeric") || min(hr) <= 0) {
stop("ahr_blinded: hr must be a vector of positive numbers.")
if (!is.numeric(hr) || min(hr) <= 0) {
stop("'hr' must be a vector of positive numbers.")
}
if (!is.vector(intervals, mode = "numeric") || min(intervals) <= 0) {
stop("ahr_blinded: intervals must be a vector of positive numbers.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi Yihui, I have a question regarding the error message output. Typically, our practice in gsDesign2 is to print the function name at the beginning of the error message. This consideration arises from the possibility that the ahr_blinded function could potentially be invoked within another function. So... I am wondering if we should keep the ahr_blinded here?

Copy link
Contributor Author

@yihui yihui Aug 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Good question. I deleted the function name in the error message because I thought it would be redundant.

f = function() foo()
foo = function() {
  stop('No!')
}
f()
#> Error in foo() : No!
foo = function() {
  stop('foo: No!')
}
f()
#> Error in foo() : foo: No!

This relates to a comment I made after #448 (comment) That is, the function name will be missing if you do.call the function.

foo = function() {
  stop('No!')
}
f = function() do.call(foo, list())
f()
#> Error in (function ()  : No!

I think this case is relatively rare (so far we have used do.call() mostly with rbind/cbind in this package). If it does happen, traceback() can be helpful to know the chain of function calls.

That said, again I don't have a strong opinion here. In general, my principle is that I'll follow the preference of the project maintainer (i.e. you in this case).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for the clear explanation; I understand your intention. I am open to this. If we choose to remove the function name at the beginning of the error message, we should review the entire gsDesign2 package, as the previous practice of gsDesign2 is to include the function name right at the start (maybe a new issue and PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, as I'm going through the whole codebase, I'll do this along the way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, @yihui! This is not urgent and thanks again for reviewing the codes!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I searched the repository for how ahr_blinded() is used. From the package code, examples, and tests, ahr_blinded() is always called directly. In other words, the function itself is never passed as an argument to another function. Thus the fact that an error comes from ahr_blinded() should be clear from the default error message, as @yihui noted above.

To demonstrate, I added stop("there was an error") to ahr_blinded() and generated the error message below:

devtools::load_all()
ahr_blinded(
  surv = survival::Surv(
    time = simtrial::ex2_delayed_effect$month,
    event = simtrial::ex2_delayed_effect$evntd
  ),
  intervals = c(4, 100),
  hr = c(1, .55),
  ratio = 1
)
## Error in ahr_blinded(surv = survival::Surv(time = simtrial::ex2_delayed_effect$month,  : 
##   there was an error

if (!is.numeric(intervals) || min(intervals) <= 0) {
stop("'intervals' must be a vector of positive numbers.")
}
if (length(intervals) != length(hr)) {
stop("ahr_blinded: the piecewise model specified hr and intervals are not aligned.")
stop("the piecewise model specified 'hr' and 'intervals' differ in length.")
}

# Set final element of "intervals" to Inf
Expand All @@ -111,11 +111,10 @@ ahr_blinded <- function(
# Compute adjustment for information
q_e <- ratio / (1 + ratio)

ans <- tibble(
tibble(
event = sum(event),
ahr = exp(-theta),
theta = theta,
info0 = sum(event) * (1 - q_e) * q_e
)
return(ans)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally I like the explicit return even though it is optional. And checking git blame, I'm pretty sure they existed before I started editing the code base. I'm happy to go along with the consensus though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a strong opinion here and can definitely use an explicit return(). Personally I use return() only when I need to return early.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LittleBeannie Do you have a preference here? As I said, I'm totally fine with reverting to the original style ans <- ...; return(ans), even though both the assignment and the explicit return() look unnecessary to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for verifying, @yihui ! I personally prefer the explicit return, as it provides a clearer message for non-advanced R programmers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. I've restored return() in 90352a0.

}
Loading