tidymodels / censored

Parsnip wrappers for survival models
https://censored.tidymodels.org/
Other
123 stars 12 forks source link

Clean-up of `$fit` slot of fitted glmnet models #266

Closed hfrick closed 12 months ago

hfrick commented 1 year ago

closes #265

But: the size of the total object is now bigger?? 🤔

To compare from the issue/main:

lobstr::obj_size(glmn_fit)
#> 112.16 kB
library(censored)
#> Loading required package: parsnip
#> Loading required package: survival
lung2 <- lung[-14, ]

glmn_fit <- proportional_hazards(penalty = 0.1) %>%
  set_engine("glmnet") %>%
  fit(Surv(time, status) ~ age + ph.ecog, data = lung2)

lobstr::obj_size(lung2)
#> 34.61 kB
lobstr::obj_size(glmn_fit)
#> 172.58 kB
lobstr::obj_size(glmn_fit$fit)
#> 9.54 kB
lobstr::obj_size(glmn_fit$fit$fit)
#> 0 B

Created on 2023-04-26 with reprex v2.0.2

Second thoughts

I think this change is the right call regardless of my surprise with the size because extract_fit_engine() expects the model to sit in model_fit$fit, not model_fit$fit$fit. Maybe something else also builds on this convention (but I haven't checked).

Tests are updated in https://github.com/tidymodels/extratests/pull/149

With main dev version

library(censored)
#> Loading required package: parsnip
#> Loading required package: survival

lung2 <- lung[-14, ]

ph_fit <- proportional_hazards(penalty = 0.1) %>%
  set_engine("glmnet") %>%
  fit(Surv(time, status) ~ age + ph.ecog, data = lung2)

extract_fit_engine(ph_fit) 
#> $fit
#> 
#> Call:  glmnet::glmnet(x = data_obj$x, y = data_obj$y, family = "cox",      weights = weights, alpha = alpha, lambda = lambda) 
#> 
#>    Df %Dev   Lambda
#> 1   0 0.00 0.225300
#> 2   1 0.21 0.205300
[snip]
#> 
#> $preproc
#> $preproc$x
#>     age ph.ecog
#> 1    74       1
#> 2    68       0
[snip]
#> 
#> $preproc$y
#>   [1]  306   455  1010+  210   883  1022+  310   361   218   166   170   654 
#>  [13]  728   567   144   613   707    61    88   301    81   624   371   394 
[snip]

Created on 2023-11-23 with reprex v2.0.2

With this PR

library(censored)
#> Loading required package: parsnip
#> Loading required package: survival

lung2 <- lung[-14, ]

ph_fit <- proportional_hazards(penalty = 0.1) %>%
  set_engine("glmnet") %>%
  fit(Surv(time, status) ~ age + ph.ecog, data = lung2)

extract_fit_engine(ph_fit)
#> 
#> Call:  glmnet::glmnet(x = data_obj$x, y = data_obj$y, family = "cox",      weights = weights, alpha = alpha, lambda = lambda) 
#> 
#>    Df %Dev   Lambda
#> 1   0 0.00 0.225300
#> 2   1 0.21 0.205300
[snip]

Created on 2023-11-23 with reprex v2.0.2

hfrick commented 1 year ago

revdep check job_name: "8d466dbc-c0f2-4516-a1f8-7092a1dfcc45"

glmnet-cleanup-training-data* > revdepcheck::cloud_summary()
ℹ Syncing results to revdep/cloud.noindex/8d466dbc-c0f2-4516-a1f8-7092a1dfcc45
ℹ Comparing results
✔ MachineShop 3.7.0                      ── E: 0     | W: 0     | N: 0    
✔ survex 1.2.0                           ── E: 0     | W: 0     | N: 0    
simonpcouch commented 12 months ago

I would also be perplexed by an increase in object size, but I'm seeing different object sizes than you for some reason. I do indeed see a decrease in object sizes with this PR🤔

With main:

library(censored)
#> Loading required package: parsnip
#> Loading required package: survival

lung2 <- lung[-14, ]

glmn_fit <- proportional_hazards(penalty = 0.1) %>%
  set_engine("glmnet") %>%
  fit(Surv(time, status) ~ age + ph.ecog, data = lung2)

lobstr::obj_size(glmn_fit)
#> 112.04 kB

lobstr::obj_size(glmn_fit$fit)
#> 63.98 kB

lobstr::obj_size(glmn_fit$fit$fit)
#> 9.49 kB

Created on 2023-11-28 with reprex v2.0.2

With this PR:

library(censored)
#> Loading required package: parsnip
#> Loading required package: survival

lung2 <- lung[-14, ]

glmn_fit <- proportional_hazards(penalty = 0.1) %>%
  set_engine("glmnet") %>%
  fit(Surv(time, status) ~ age + ph.ecog, data = lung2)

lobstr::obj_size(glmn_fit)
#> 111.13 kB

lobstr::obj_size(glmn_fit$fit)
#> 9.49 kB

lobstr::obj_size(glmn_fit$fit$fit)
#> 0 B

Created on 2023-11-28 with reprex v2.0.2

Regardless, now transitioning into sinking my teeth into the substance of the PR🏄

simonpcouch commented 12 months ago

Possibly related oddities: https://github.com/tidymodels/stacks/pull/117.

github-actions[bot] commented 11 months ago

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.