cmu-delphi / epipredict

Tools for building predictive models in epidemiology.
https://cmu-delphi.github.io/epipredict/
Other
8 stars 9 forks source link

refactor: use checkmate for arg validation #286

Closed dshemetov closed 8 months ago

dshemetov commented 8 months ago

Related to https://github.com/cmu-delphi/epiprocess/issues/403.

I took the safe approach of changing the internals of the arg functions to use checkmate, that way we can rely on the existing unit tests and don't need to change any other epipredict code. The functions look a lot simpler and more unified now. We could eventually get rid of these wrappers and just invoke check functions directly, but that would take more refactoring work.

dshemetov commented 8 months ago

Good question right, I brought back the handle_arg_list since it handled grabbing names well. Checkmate does quite well with preserving variable names in errors.

r$> r1 <- epi_recipe(x) %>%
          step_epi_ahead(death_rate, ahead = 3.6) %>%
          step_epi_lag(death_rate, lag = 1.9)
Error in (function (name, value)  : 
  Assertion on 'ahead' failed: Must be of type 'integerish', but element 1 is not close to an integer.

r$> r2 <- epi_recipe(x) %>%
          step_epi_ahead(death_rate, ahead = 7) %>%
          step_epi_lag(death_rate, lag = -7)
Error in (function (name, value)  : 
  Assertion on 'lag' failed: Element 1 is not >= 0.

r$> step_growth_rate(r, value, role = 1)
Error in (function (name, value)  : 
  Assertion on 'role' failed: Must be of type 'character', not 'double'.

r$> step_growth_rate(r, value, horizon = 0)
Error in (function (name, value)  : 
  Assertion on 'horizon' failed: Element 1 is not >= 1.
dshemetov commented 8 months ago

FWIW, traceback() can lead the user there

r$> r1 <- epi_recipe(x) %>%
              step_epi_ahead(death_rate, ahead = 3.6) %>%
              step_epi_lag(death_rate, lag = 1.9)
Error in (function (name, value)  : 
  Assertion on 'ahead' failed: Must be of type 'integerish', but element 1 is not close to an integer.

r$> traceback()
15: stop(simpleError(sprintf(msg, ...), call.))
14: mstop("Assertion on '%s' failed: %s.", var.name, res, call. = sys.call(-2L))
13: makeAssertion(x, res, .var.name, add)
12: assert_integerish(value, null.ok = allow_null, lower = 0, any.missing = FALSE, 
        .var.name = name) at utils-arg.R#63
11: (function (name, value) 
    {
        assert_integerish(value, null.ok = allow_null, lower = 0, 
            any.missing = FALSE, .var.name = name)
    })(dots[[1L]][[1L]], dots[[2L]][[1L]])
10: mapply(.f, .x, .y, MoreArgs = list(...), SIMPLIFY = FALSE) at compat-purrr.R#72
9: map2(.x, .y, .f, ...) at compat-purrr.R#15
8: walk2(names, values, .tests) at utils-arg.R#10
7: handle_arg_list(..., .tests = function(name, value) {
       assert_integerish(value, null.ok = allow_null, lower = 0, 
           any.missing = FALSE, .var.name = name)
   }) at utils-arg.R#62
6: arg_is_nonneg_int(ahead) at step_epi_shift.R#126
5: step_epi_ahead(., death_rate, ahead = 3.6)
4: inherits(x, "epi_recipe") at epi_recipe.R#221
3: is_epi_recipe(recipe) at step_epi_shift.R#64
2: step_epi_lag(., death_rate, lag = 1.9)
1: epi_recipe(x) %>% step_epi_ahead(death_rate, ahead = 3.6) %>% 
       step_epi_lag(death_rate, lag = 1.9)
dajmcdon commented 8 months ago

True, though it would be nice if Error in (function (name, value) : said Error in step_epi_ahead() : instead.