potential patch for markmfredrickson/RItools#124

[adamrauh]

Regarding #124, I think the warning suppression might not be from the finally argument, but from resetting the options. I also think having a tryCatch() block as currently structured + having a call to on.exit() is redundant, since there's no real additional error/warning handling happening, but let me know if I'm missing any important context there.

I pulled the following example from test.balanceTest.R, specifically test_that("Consistency between lm() and balTest()") in order to generate a warning:

n <- 100
dta.all <- data.frame(z = rep(c(1,1,0,0), n/4),
                      x1 = rnorm(n),
                      blk = rep(1:(n/4), each = 4),
                      size = rpois(n = n, lambda = 200))

dta.lost <- dta.all[3:n, ]

balanceTest(z ~ x1 + strata(blk) - 1,
            data = dta.lost,
            unit.weights = size)

This should should generate: Warning message: In makeDesigns(fmla, data) : 1 levels of survival::strata(blk) did not include both treated and control units

I used this example to try and isolate the behavior identified in the issue:

This version of withOptions() will print the warning message

withOptions <- function(optionsToChange, fun) {
  oldOpts <- options()
  options(optionsToChange)
  fun()
}

... but this one will not:

withOptions <- function(optionsToChange, fun) {
  oldOpts <- options()
  on.exit(options(oldOpts))
  options(optionsToChange)
  fun()
}

This one will:

withOptions <- function(optionsToChange, fun) {
  oldOpts <- options()
  options(optionsToChange)
  tryCatch(fun())
}

...but this one will not:

withOptions <- function(optionsToChange, fun) {
  oldOpts <- options()
  options(optionsToChange)
  tryCatch(fun(), finally = options(oldOpts))
}

So, the warning suppression seems to be related to the line options(oldOpts).

Rather than blanket resetting all of the options, resetting just the options that were changed seems to solve the warning suppression (removing the redundant on.exit() as well), as the following code:

withOptions <- function(optionsToChange, fun) {
  oldOpts <- options()
  options(optionsToChange)
  # store the old values of the options, just for the options that were changed
  old.opt.values <- list()
  for (i in 1:length(optionsToChange)) {
    old.opt.values[[names(optionsToChange)[i]]] <- oldOpts[[names(optionsToChange)[i]]]
  }
  tryCatch(fun(), finally = options(old.opt.values))
}

generates warnings when running the test code above:

> balanceTest(z ~ x1 + strata(blk) - 1,
+             data = dta.lost,
+             unit.weights = size)
     strata():       blk                                         
     stat      Treatment Control adj.diff std.diff    z          
vars                                                             
x1                -0.082   0.131   -0.213   -0.2    -1           
---Overall Test---
    chisquare df p.value
blk       1.6  2    0.45
---
Signif. codes:  0 ‘***’ 0.001 ‘** ’ 0.01 ‘*  ’ 0.05 ‘.  ’ 0.1 ‘   ’ 1 
Warning message:
In makeDesigns(fmla, data) :
  1 levels of survival::strata(blk)  did not include both treated and control units

This seems to work and doesn't break any tests (from running devtools::check()). But, admittedly, I'm not totally sure why. In versions with options(oldOpts), calling warnings() after running balanceTest() returns nothing, but calling it after the versions with options(old.opt.values) returns the expected warning. So, somehow they're getting cleared in the process of resetting all of the options.

[benthestatistician]

Nice detective work here! Please see embedded comment for some suggestions.

[adamrauh]

Thanks, Ben! I incorporated both suggestions into another commit. I agree your proposal in (1) is cleaner -- and does in fact accomplish the same thing.