Skip to content

add case_weights argument to workflow_set - #82

Merged
simonpcouch merged 18 commits into
mainfrom
case-weights-v2
May 31, 2022
Merged

add case_weights argument to workflow_set#82
simonpcouch merged 18 commits into
mainfrom
case-weights-v2

Conversation

@simonpcouch

@simonpcouch simonpcouch commented May 19, 2022

Copy link
Copy Markdown
Contributor
library(workflowsets)
library(parsnip)

# setup
lr_spec <- linear_reg() %>% set_engine("lm")
knn_spec <- nearest_neighbor() %>%
   set_engine("kknn") %>%
   set_mode("regression")

cars <-
   mtcars %>%
   dplyr::mutate(
      wts = hardhat::importance_weights(runif(nrow(.))),
      non_wts = runif(nrow(.))
   )

# when done right:
workflow_set(
   list(reg = mpg ~ ., nonlin = mpg ~ wt + 1 / sqrt(disp)),
   list(lm = lr_spec),
   case_weights = wts
) %>%
   workflow_map(
      "fit_resamples",
      resamples = rsample::vfold_cv(cars, v = 5)
   )
#> # A workflow set/tibble: 2 × 4
#>   wflow_id  info             option    result   
#>   <chr>     <list>           <list>    <list>   
#> 1 reg_lm    <tibble [1 × 4]> <opts[1]> <rsmp[x]>
#> 2 nonlin_lm <tibble [1 × 4]> <opts[1]> <rsmp[x]>

# specify a column that is not case weights:
workflow_set(
   list(reg = mpg ~ ., nonlin = mpg ~ wt + 1 / sqrt(disp)),
   list(lm = lr_spec),
   case_weights = non_wts
) %>%
   workflow_map(
      "fit_resamples",
      resamples = rsample::vfold_cv(cars, v = 5)
   )
#> Error:
#> ! The supplied `case_weights` argument 'non_wts' is not a case weights column. See `?workflow_set` for more information.
#> Execution stopped; returning current results
#> # A workflow set/tibble: 2 × 4
#>   wflow_id  info             option    result    
#>   <chr>     <list>           <list>    <list>    
#> 1 reg_lm    <tibble [1 × 4]> <opts[1]> <list [0]>
#> 2 nonlin_lm <tibble [1 × 4]> <opts[1]> <list [0]>

# specify an engine that allows case weights:
workflow_set(
   list(reg = mpg ~ ., nonlin = mpg ~ wt + 1 / sqrt(disp)),
   list(lm = knn_spec),
   case_weights = wts
)
#> Warning: Case weights are not enabled by the underlying model implementation for the following engine(s): kknn.
#> 
#> The `case_weights` argument will be ignored for specifications using that engine.
#> # A workflow set/tibble: 2 × 4
#>   wflow_id  info             option    result    
#>   <chr>     <list>           <list>    <list>    
#> 1 reg_lm    <tibble [1 × 4]> <opts[0]> <list [0]>
#> 2 nonlin_lm <tibble [1 × 4]> <opts[0]> <list [0]>

Created on 2022-05-26 by the reprex package (v2.0.1)

probably more informative to refer to the engine rather than model spec in the warning.

also, ignores the case weights entirely if they aren't implemented for one model: not sure whether ignoring entirely or just for some models is more sound.
@simonpcouch simonpcouch linked an issue May 19, 2022 that may be closed by this pull request
@simonpcouch
simonpcouch marked this pull request as ready for review May 20, 2022 17:47
@simonpcouch

Copy link
Copy Markdown
Contributor Author

cc @DavisVaughan—if you're game, let's review this PR in pairing on Monday. Would like to watch your process and also get a sense for whether there's a better way to subset into these objects in the case weights logic. I think some of the code points toward need for getters/indicators like has_case_weights or get_case_weights_name.

Comment thread R/workflow_map.R Outdated
Comment thread R/workflow_set.R
Comment thread DESCRIPTION Outdated
Comment thread R/workflow_set.R

@DavisVaughan DavisVaughan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review which we will continue on Tuesday

Comment thread DESCRIPTION Outdated
Comment thread NEWS.md Outdated

@DavisVaughan DavisVaughan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good with a few comments

Comment thread R/misc.R Outdated
Comment thread R/misc.R Outdated
Comment thread R/workflow_map.R Outdated
Comment thread R/workflow_map.R Outdated
Comment thread R/workflow_set.R Outdated
Comment thread R/workflow_set.R Outdated
Comment thread R/workflow_set.R Outdated
Comment thread tests/testthat/test-workflow_set.R Outdated
Comment thread tests/testthat/test-workflow_set.R Outdated
in response to davis' review. also, makes tests more modular, updates snapshots, makes better use of rlang machinery, uses tidy style for function names.
@simonpcouch

Copy link
Copy Markdown
Contributor Author

Much appreciated, @DavisVaughan! Rest of changes resolved in 639c2ea.

Comment thread tests/testthat/test-workflow_set.R Outdated
@simonpcouch
simonpcouch merged commit 13342ad into main May 31, 2022
@simonpcouch
simonpcouch deleted the case-weights-v2 branch May 31, 2022 17:08
@github-actions

Copy link
Copy Markdown

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.

@github-actions github-actions Bot locked and limited conversation to collaborators Jun 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

case weight support

3 participants