adibender / pammtools

Piece-wise exponential Additive Mixed Modeling tools
https://adibender.github.io/pammtools/
Other
47 stars 11 forks source link

Fix breaking changes in dplyr dependency #101

Closed adibender closed 5 years ago

adibender commented 5 years ago

This just in from romain:

This is an automated email to let you know that:

This release is a follow up to the 0.8.0 release

I need your help to keep pammtools and dplyr working together smoothly. In the next days, can you please:

  1. Read about the changes to dplyr at https://github.com/tidyverse/dplyr/blob/master/NEWS.md. This page includes a list of breaking changes, the reasoning behind them, and to how to update your code.

  2. Carefully inspect the failing checks listed at the bottom of this email.

  3. For each failing check, either update your package, or tell me that I have a bug. If you have made changes to your package, please submit an update to CRAN before April 22.

If you have discovered a bug in dplyr, please file an issue (ideally with a small reprex that illustrates the problem) at https://github.com/tidyverse/dplyr/issues. If you're not sure whether or not you've found a bug, please an issue and we'll help you figure it out. Breaking changes that are not listed qualify as bugs.

Please respond to this message if you have any questions.

Thanks,

Romain Francois

== CHECK RESULTS ========================================

adibender commented 5 years ago

@fabian-s Are you able to reproduce this? Installed the current dplyr from GitHub and ran test + checks but didn't get any errors...

fabian-s commented 5 years ago

Nope.

After devtools::install_github("tidyverse/dplyr", ref = "r-0.8.1"), I get

> devtools::test()
Loading pammtools
Testing pammtools
✔ | OK F W S | Context
✔ | 70   1   | Convenience functions for calculation of hazard and similar [6.9 s]
───────────────────────────────────────────────────────────────────────────────
test-add-functions.R:228: warning: hazards and CI positive for type response
funs() is soft deprecated as of dplyr 0.8.0
please use list() instead

  # Before:
  funs(name = f(.))

  # After: 
  list(name = ~ f(.))
This warning is displayed once per session.
───────────────────────────────────────────────────────────────────────────────
✔ | 15       | Test as_ped functions [0.4 s]
✔ |  4       | Test cumulative coefficients functionality [0.4 s]
✔ | 24       | Cumulative effects (of time-dependent covariates) [1.8 s]
✔ |  7       | Formula utility functions
✔ | 15       | Interal info and median and modus information [0.2 s]
✔ |  1       | mgcv convenience functions [0.3 s]
✔ | 42       | create newdata [0.8 s]
✔ | 24       | Simple transformation to PED data [0.2 s]
✔ |  2       | Test simulation functions
✔ | 12       | Test formula special. [0.2 s]
✔ |  6       | Transformation of longitudinal covariates to functional covariates
✔ | 32       | Transformation with TDC [0.6 s]
✔ | 37   3   | Tidyverse methods for specific classes [0.2 s]
───────────────────────────────────────────────────────────────────────────────
test-tidyverse-S3methods.R:13: warning: ped class is preserved after dplyr operations
distinct_() is deprecated. 
Please use distinct() instead

The 'programming' vignette or the tidyeval book can help you
to program with distinct() : https://tidyeval.tidyverse.org
This warning is displayed once per session.

test-tidyverse-S3methods.R:18: warning: ped class is preserved after dplyr operations
select_() is deprecated. 
Please use select() instead

The 'programming' vignette or the tidyeval book can help you
to program with select() : https://tidyeval.tidyverse.org
This warning is displayed once per session.

test-tidyverse-S3methods.R:20: warning: ped class is preserved after dplyr operations
rename_() is deprecated. 
Please use rename() instead

The 'programming' vignette or the tidyeval book can help you
to program with rename() : https://tidyeval.tidyverse.org
This warning is displayed once per session.
───────────────────────────────────────────────────────────────────────────────

══ Results ════════════════════════════════════════════════════════════════════
Duration: 12.2 s

OK:       291
Failed:   0
Warnings: 4
Skipped:  0
> sessioninfo::package_info()
 ! package     * version  date       lib source                              
[...]         
   dplyr         0.8.1    2019-04-12 [1] Github (tidyverse/dplyr@55de39a)    
[...]    
 P pammtools   * 0.1.11   2019-04-12 [?] Github (adibender/pammtools@35d0273)
[...]

all other packages are at their respective CRAN (R 3.5.3) versions, I re-installed R and update d all packages yesterday.

adibender commented 5 years ago

hm, maybe they ran with an option that sets warnings to errors, but I don't recognise anything pammtools specific in their output...

adibender commented 5 years ago

Ok, going back to the release version of pammtools I get:

1: .Call(`_dplyr_mutate_impl`, df, dots, caller_env)                                                                                  
 2: mutate_impl(.data, dots, caller_env())                                                                                             
 3: mutate.tbl_df(.tbl, !(!(!funs)))                                                                                                   
 4: mutate(.tbl, !(!(!funs)))                                                                                                          
 5: mutate_at(., .vars = vars(-one_of(c("tstart", "tend", "intlen",     "intmid", "interval"))), .funs = ~0)                           
 6: function_list[[i]](value)                                                                                                          
 7: freduce(value, `_function_list`)                                                                                                   
 8: `_fseq`(`_lhs`)                                                                                                                    
 9: eval(quote(`_fseq`(`_lhs`)), env, env)                                                                                             
10: eval(quote(`_fseq`(`_lhs`)), env, env)                                                                                             
11: withVisible(eval(quote(`_fseq`(`_lhs`)), env, env))                                                                                
12: data %>% mutate_at(.vars = vars(-one_of(c("tstart", "tend", "intlen",     "intmid", "interval"))), .funs = ~0) %>% add_cumu_hazard(
model) %>%     mutate(method = class(model)[1], variable = "(Intercept)") %>%     rename(time = "tend") %>% select(one_of(c("method", "
variable",     "time", "cumu_hazard", "cumu_lower", "cumu_upper")))                                                                    
13: get_cumu_coef_baseline(data, model)                                                                                                
14: cumu_coef(data, model, quo_name(sym(.)), ...)                                                                                      
15: .f(.x[[i]], ...)                                                                                                                   
16: .Call(map_impl, environment(), ".x", ".f", "list")                                                                                 
17: map(terms, ~cumu_coef(data, model, quo_name(sym(.)), ...))                                                                         
18: eval(lhs, parent, parent)                                                     
19: eval(lhs, parent, parent)
20: map(terms, ~cumu_coef(data, model, quo_name(sym(.)), ...)) %>%     bind_rows()
21: get_cumu_coef.gam(pam, tumor_ped, terms = c("(Intercept)", "age"))
22: get_cumu_coef(pam, tumor_ped, terms = c("(Intercept)", "age"))
23: eval(code, test_env)
24: eval(code, test_env)
25: withCallingHandlers({    eval(code, test_env)    if (!handled && !is.null(test)) {        skip_empty()    }}, expectation = handle_expectation, skip = handle_skip, warning = handle_warning,     message = handle_message, error = handle_error)
26: doTryCatch(return(expr), name, parentenv, handler)
27: tryCatchOne(expr, names, parentenv, handlers[[1L]])
28: tryCatchList(expr, names[-nh], parentenv, handlers[-nh])
29: doTryCatch(return(expr), name, parentenv, handler)
30: tryCatchOne(tryCatchList(expr, names[-nh], parentenv, handlers[-nh]),     names[nh], parentenv, handlers[[nh]])
31: tryCatchList(expr, classes, parentenv, handlers)
32: tryCatch(withCallingHandlers({    eval(code, test_env)    if (!handled && !is.null(test)) {        skip_empty()    }}, expectation
= handle_expectation, skip = handle_skip, warning = handle_warning,     message = handle_message, error = handle_error), error = handle_fatal,     skip = function(e) {    })
33: test_code(desc, code, env = parent.frame())
34: test_that("Cumulative coefficients work", {    data("tumor")    tumor <- tumor %>% dplyr::slice(1:100)    tumor_ped <- tumor %>% as_ped(formula = Surv(days, status) ~         age + complications)    pam <- mgcv::gam(formula = ped_status ~ s(tend, by = as.ordered(complications)) +         s(tend, by = age), data = tumor_ped, family = poisson(),         offset = offset)    cumu_coef <- get_cumu_coef(pam, tumor_ped, terms = c("age",         "complications"))    expect_data_frame(cumu_coef, nrows = 82L, ncols = 6L)    cumu_coef_pam <- get_cumu_coef(pam, tumor_ped, terms = c("(Intercept)",         "age"))    expect_data_frame(cumu_coef_pam, nrows = 82L, ncols = 6L)
 library(timereg)    aalen <- aalen(Surv(days, status) ~ age + complications,         data = tumor)    cumu_coef_aalen <- get_cumu_coef(aalen, terms = c("age",         "complications"))    expect_data_frame(cumu_coef_aalen, nrows = 86L, ncols = 6L)    cox.aalen <- cox.aalen(Surv(days, status) ~ age + prop(complications),         data = tumor)    cumu_coef_cox.aalen <- get_cumu_coef(cox.aalen, terms = c("age"))    expect_data_frame(cumu_coef_cox.aalen, nrows = 43, ncols = 6L)})
35: eval(code, test_env)
36: eval(code, test_env)
37: withCallingHandlers({    eval(code, test_env)    if (!handled && !is.null(test)) {        skip_empty()    }}, expectation = handle_expectation, skip = handle_skip, warning = handle_warning,     message = handle_message, error = handle_error)
38: doTryCatch(return(expr), name, parentenv, handler)
39: tryCatchOne(expr, names, parentenv, handlers[[1L]])
40: tryCatchList(expr, names[-nh], parentenv, handlers[-nh])
41: doTryCatch(return(expr), name, parentenv, handler)
42: tryCatchOne(tryCatchList(expr, names[-nh], parentenv, handlers[-nh]),     names[nh], parentenv, handlers[[nh]])
43: tryCatchList(expr, classes, parentenv, handlers)
44: tryCatch(withCallingHandlers({    eval(code, test_env)    if (!handled && !is.null(test)) {        skip_empty()    }}, expectation
= handle_expectation, skip = handle_skip, warning = handle_warning,     message = handle_message, error = handle_error), error = handle_fatal,     skip = function(e) {    })
45: test_code(NULL, exprs, env)
46: source_file(path, new.env(parent = env), chdir = TRUE, wrap = wrap)
47: force(code)
48: with_reporter(reporter = reporter, start_end_reporter = start_end_reporter,     {        lister$start_file(basename(path))        source_file(path, new.env(parent = env), chdir = TRUE,             wrap = wrap)        end_context()    })
49: FUN(X[[i]], ...)
50: lapply(paths, test_file, env = env, reporter = current_reporter,     start_end_reporter = FALSE, load_helpers = FALSE, wrap = wrap)
51: force(code)
52: with_reporter(reporter = current_reporter, results <- lapply(paths,     test_file, env = env, reporter = current_reporter, start_end_reporter = FALSE,     load_helpers = FALSE, wrap = wrap))
53: test_files(paths, reporter = reporter, env = env, stop_on_failure = stop_on_failure,     stop_on_warning = stop_on_warning, wrap =
wrap)
54: (function (path, filter = NULL, reporter = default_reporter(),     env = test_env(), ..., encoding = "unknown", load_helpers = TRUE,     stop_on_failure = FALSE, stop_on_warning = FALSE, wrap = TRUE) {    if (!missing(encoding) && !identical(encoding, "UTF-8")) {
     warning("`encoding` is deprecated; all files now assumed to be UTF-8",             call. = FALSE)    }    if (load_helpers) {
   source_test_helpers(path, env)    }    source_test_setup(path, env)    on.exit(source_test_teardown(path, env), add = TRUE)    withr::local_envvar(list(R_TESTS = "", TESTTHAT = "true"))    if (identical(Sys.getenv("NOT_CRAN"), "true")) {        withr::local_options(list(lifecycle_verbose_retirement = TRUE))    }    paths <- find_test_scripts(path, filter, ...)    test_files(paths, reporter = reporter, env = env, stop_on_failure = stop_on_failure,         stop_on_warning = stop_on_warning, wrap = wrap)})("/home/andreasb/Dropbox/GitHub/pammtools/tests/testthat",     filter = NULL, env = <environment>, load_helpers = FALSE)
55: do.call(testthat::test_dir, testthat_args)
56: force(code)
57: withr::with_envvar(r_env_vars(), do.call(testthat::test_dir,     testthat_args))
58: force(code)
59: withr::with_options(c(useFancyQuotes = FALSE), withr::with_envvar(r_env_vars(),     do.call(testthat::test_dir, testthat_args)))
60: test()
adibender commented 5 years ago

Ok, so there was a bug in get_cumu_coef_baseline which didn't show b/c I accidentally removed the respective test. Partly, because the I don't understand what the "cumulative coefficient" for the "(Intercept)" is supposed to be. Right now the "PAM" version simply returns the cumulative hazard with all covariates set to 0/reference category, which is not the same as the cumulative coefficient for the Intercept returned for Aalen models. The former is always positive, while the latter can be negative