Location of functions for root solving [lower priority]

[fb-elong]

Place to discuss comments in #229

I would appreciate it if you could make one small update: moving their function definitions outside of the exported functions.

[nanxstats]

Following the discussion today: the reason that the inner function is harder to move outside the outer function: the inner function is using variables outside of it's scope but within the outer function's scope. To move it out, additional arguments need to be added.

[elong0527]

Removed my assignment as no short-term plan to complete it.

[nanxstats]

@jdblischak do you mind helping with this when you got some time? Not urgent.

The goal is to move the inline functions event_diff() and get_combo_power() out of the upper-level function:

gsDesign2/R/expected_time.R

Line 110 in 24fdc77

event_diff <- function(x) {

gsDesign2/R/gs_design_combo.R

Line 194 in 4917d25

get_combo_power <- function(n, beta, ...) {

They are used in uniroot() calls.

This would drastically improve the rigorousness of these functions, because currently they used variables not defined in the function signature, thus R will search for those variables in upper level environments, which is considered dangerous. Defining them outside would enforce all invoked variables properly defined in the function signature. This would also avoid these being repeatedly redefined every time the upper-level functions are called which can be a performance hog.

[jdblischak]

event_diff() was straightforward, so I started with it. See #312

I'm a bit more cautious about get_combo_power() because the call to uniroot() in gs_design_combo() is more complex. It allows passing arguments via ... in the top-level function signature all the way to mvtnorm::pmvnorm() via the uniroot() call

gsDesign2/R/gs_design_combo.R

Line 26 in 59e8a03

#' @param ... Additional parameters passed to [mvtnorm::pmvnorm].

gsDesign2/R/gs_design_combo.R

Line 225 in 59e8a03

sample_size <- uniroot(get_combo_power, c(1, n_upper_bound), extendInt = "yes", beta = beta, ...)$root

However, as a quick test, I deleted the ... from the uniroot() call and ran the tests. Everything passed, which means this feature isn't being tested, and thus we wouldn't know if my change broke this behavior. My plan is to write a test first. If anyone knows of a realistic use case for passing an argument to mvtnorm::pmvnorm(), I'd appreciate any suggestions

[nanxstats]

So get_combo_power() accepts ..., and gs_prob_combo() also accepts ... in the function signature. However, in the definition of gs_prob_combo(), ... is not used at all.

Then it would be important to check in with the original author about the intention. Since ... was not used, should it be deleted from all these function signatures and calls to avoid confusion?

In general, it's worthwhile to review the call chain in gs_design_combo(): ... is being passed to multiple function calls. I wonder if they are all necessary - gs_utility_combo() seems to have the same issue as above.

[nanxstats]

Hey @elong0527 I can't recall who wrote this part, could you point us to the author to clarify the ... usage here?

[elong0527]

I guess I am the author, agree we should avoid ... from multiple call.

The intention is to use ... in the pmvnorm_combo function. We can check if that's the final place of using ..., if not we can skip.

[jdblischak]

I confirmed that currently the ... are not passed all the way to mvtnorm::pmvnorm()

library("gsDesign2")
debugonce(mvtnorm::pmvnorm)
gs_design_combo(seed = 1)
Browse[2]> print(seed)
NULL

[jdblischak]

However, this was easily fixed by adding ... to a few calls to pmvnorm_combo()

library("gsDesign2")
debugonce(mvtnorm::pmvnorm)
gs_design_combo(seed = 1)
Browse[2]> print(seed)
[1] 1

[nanxstats]

A quick note on our discussion today: we agreed that it's important to keep those ... and ensure they are being correctly passed to all lower level functions. John will

• Fix the current minor issues where ... was forgotten to be added (Pass args via ... to mvtnorm::pmvnorm() #315).
• Add new tests that verify if the mvtnorm::pmvnorm() parameters are actually working when specifying different inputs such as maxpts and tolerance in the exported functions.
• Move that internal function outside (and make the function signature proper).