Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not allow fitted recipes #179

Closed
topepo opened this issue Sep 21, 2022 · 5 comments · Fixed by #202
Closed

Do not allow fitted recipes #179

topepo opened this issue Sep 21, 2022 · 5 comments · Fixed by #202

Comments

@topepo
Copy link
Member

topepo commented Sep 21, 2022

Another thing that I've seen is that people prep a recipe then use add_recipe(). Can we error when that happens and add something to the man file for add_recipe()?

library(tidymodels)

training <- mtcars[1:20, ]
testing <- mtcars[21:32, ]

model <- linear_reg() %>%
  set_engine("lm")

workflow <- workflow() %>%
  add_model(model)

recipe <- recipe(mpg ~ cyl + disp, training) %>%
  step_log(disp) %>% 
  prep()

map_lgl(recipe$steps, is_trained)
#> [1] TRUE

workflow <- add_recipe(workflow, recipe)

fit_workflow <- fit(workflow, training)

Created on 2022-09-21 by the reprex package (v2.0.1)

@DavisVaughan
Copy link
Member

DavisVaughan commented Sep 21, 2022

Maybe recipes::is_trained() should do all(map_lgl(x$steps, is_trained)) if given a recipe. Then workflows can do

if (recipes::is_trained(recipe)) {
  abort("Can't add a trained recipe to a workflow.")
}

I wouldn't worry too much about partially trained recipes

@simonpcouch
Copy link
Contributor

Unfortunately the all(map_lgl(rec$steps, is_trained)) approach wouldn't do the trick either, as in the no-steps case:

library(recipes)

rec <- recipe(mpg ~ cyl, mtcars)
rec_trained <- recipe(mpg ~ cyl, mtcars) %>% prep()
  
all(purrr::map_lgl(rec$steps, is_trained))
#> [1] TRUE
all(purrr::map_lgl(rec_trained$steps, is_trained))
#> [1] TRUE

From the print.recipe() method, though, it looks like "tr_info" %in% names(x) is a flag we could look to:

"tr_info" %in% names(rec)
#> [1] FALSE
"tr_info" %in% names(rec_trained)
#> [1] TRUE

@EmilHvitfeldt, would you game for a PR that transitions is_trained() to a method with a recipe and default method?

Created on 2023-05-05 with reprex v2.0.2

@EmilHvitfeldt
Copy link
Member

Are you looking for fully_trained()?

library(recipes)

rec <- recipe(mpg ~ cyl, mtcars)
rec_trained <- recipe(mpg ~ cyl, mtcars) %>% prep()

fully_trained(rec)
#> [1] FALSE

fully_trained(rec_trained)
#> [1] TRUE

@simonpcouch
Copy link
Contributor

Perfect, thanks so much @EmilHvitfeldt!

@github-actions
Copy link

This issue 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 May 23, 2023
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 a pull request may close this issue.

4 participants