-
Notifications
You must be signed in to change notification settings - Fork 25
Check compatibility of model spec and tailor #304
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds compatibility validation between parsnip model specifications and tailor postprocessors in workflows. The validation ensures that model modes (regression, classification, etc.) are compatible with tailor types (regression, binary, multiclass) before adding them to a workflow.
- Adds compatibility checks in
add_tailor(),add_model(), andworkflow()functions - Implements validation logic to prevent incompatible model-tailor combinations
- Adds comprehensive test coverage for the new validation functionality
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| R/workflow.R | Updates error message format and adds validation call to add_postprocessor() |
| R/post-action-tailor.R | Implements core compatibility validation logic between model specs and tailors |
| R/fit-action-model.R | Adds validation when adding models to workflows with existing tailors |
| tests/testthat/test-workflow.R | Tests compatibility validation and postprocessor validation in main workflow function |
| tests/testthat/test-post-action-tailor.R | Tests compatibility validation when adding tailors to workflows |
| tests/testthat/test-fit-action-model.R | Tests compatibility validation when adding models to workflows with tailors |
| tests/testthat/test-extract.R | Updates test to use compatible model type for existing tailor |
| tests/testthat/_snaps/*.md | Snapshot files for error message testing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
simonpcouch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, so helpful to have that description as a refresher. Thank you for the thoroughness!
Looks great.
| check_dots_empty() | ||
|
|
||
| if (is_tailor(postprocessor)) { | ||
| # check compatibility here for the right call in the error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super helpful comment
| recipes::step_pca(recipes::all_predictors(), threshold = hardhat::tune()) | ||
| ) | ||
| wflow <- add_model(wflow, parsnip::linear_reg()) | ||
| wflow <- add_model(wflow, parsnip::logistic_reg()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh finding it very encouraging that this successfully surfaced an issue in tests
Co-authored-by: Simon P. Couch <[email protected]>
|
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. |
closes #291
Parsnip
model_specs can have a mode of"classification","regression","censored regression"or"unknown". When adding a model spec to a workflow, the mode cannot be unknown anymore.Tailors can have a
typeof"regression","binary","multiclass"or"unknown". There is currently no way for the user to set the type directly, so workflows does not require the tailor to have a type other than unknown.Tailors start with an unknown mode (via
tailor::tailor()) and then the type gets inferred from the adjustments added.When we add a tailor to a workflow, it will have
type = "unknown"only if it's a tailor without any adjustments, or all adjustments areadjust_predictions_custom().type = "binary"for all classification problems, including multi-class. Thetypeonly gets updated duringfit.tailor().type = "regression"Once it comes to fitting a tailor, we have the response, so we can update the tailor type
"unknown"to one of the other ones"binary"to"multiclass"if appropriateThis PR checks what we can when specifying the workflow, in
add_tailor(),add_model(), andworkflows().In those situations where
fit.tailor()changes the tailor type, we can't really do anything earlier, or are fine already:"unknown"tailor type gets changed, it was either a plain tailor (which is fine) or only custom adjustments (which we can't check)."binary"to"multiclass", we already checked that the model mode is"classification".