Skip to content

Remove intercept coming from workflows based on engine encodings #1033

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Dec 7, 2023
Merged

Remove intercept coming from workflows based on engine encodings #1033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e479ba0
for intercepts coming from workflows (details in the PR description)
hfrick Dec 1, 2023
c166281
bump version for extratests
hfrick Dec 1, 2023
b54dcd1
update NEWS
hfrick Dec 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
Package: parsnip
Title: A Common API to Modeling and Analysis Functions
Version: 1.1.1.9003
Version: 1.1.1.9004
Authors@R: c(
person("Max", "Kuhn", , "max@posit.co", role = c("aut", "cre")),
person("Davis", "Vaughan", , "davis@posit.co", role = "aut"),
Expand Down
3 changes: 3 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@

* When computing censoring weights, the resulting vectors are no longer named (#1023).

* Fixed a bug in the integration with workflows where using a model formula with a formula preprocessor could result in a double intercept (#1033).


# parsnip 1.1.1

* Fixed bug where prediction on rank deficient `lm()` models produced `.pred_res` instead of `.pred`. (#985)
Expand Down
4 changes: 4 additions & 0 deletions R/convert_data.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@
rlang::abort("`composition` should be either 'data.frame' or 'matrix'.")
}

if (remove_intercept) {
data <- data[, colnames(data) != "(Intercept)", drop = FALSE]
Copy link
Member

Choose a reason for hiding this comment

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

Should we use a a regex or more general method for determining what should be taken out?

I think that it is unlikely to happen but, depending on how you make the data, you could get multiple columns with nearly the same names:

mat_1 <- model.matrix( ~ ., data = mtcars[, 2:4])
colnames(mat_1)
#> [1] "(Intercept)" "cyl"         "disp"        "hp"

mat_2 <- model.matrix( ~ ., data = as.data.frame(mat_1))
colnames(mat_2)
#> [1] "(Intercept)"   "`(Intercept)`" "cyl"           "disp"         
#> [5] "hp"

colnames(mat_2) != "(Intercept)"
#> [1] FALSE  TRUE  TRUE  TRUE  TRUE

Created on 2023-12-07 with reprex v2.0.2

Maybe something like:

col_names <- c("(Intercept)", "`(Intercept)`", "cyl", "disp", "hp")
grepl("^[[:punct:]]+Intercept[[:punct:]]+", col_names)
#> [1]  TRUE  TRUE FALSE FALSE FALSE

Created on 2023-12-07 with reprex v2.0.2

Copy link
Member Author

@hfrick hfrick Dec 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I did run into this.

I think the only time that we would trigger this (as opposed to somebody including a predictor named (Intercept) or the like) is when we send the intercept coming from workflows into model.matrix(). We only do that in .convert_form_to_xy_fit().

We could let that happen and just clean up "more thoroughly"via such a regrex afterwards. That is not a good idea because then that intercept-as-predictor is stored in the terms which are re-used during prediction. That's what caused tidymodels/censored#272. Therefore, we need to remove the intercept coming from workflows before we make the terms.

As for generally beefing up and using the regrex instead of the direct name match as we have it now: I'm inclined to leave it as is because I think it might be helpful if such a case errors rather than "gets fixed". I am relatively confident that we now properly clean up after workflows. If not, I'd like to see it fail and investigate. If there is an additional intercept to clean up via the regrex that's not coming from workflows, maybe that should error too?

}

## Assemble model.frame call from call arguments
mf_call <- quote(model.frame(formula, data))
mf_call$na.action <- match.call()$na.action # TODO this should work better
Expand Down
9 changes: 9 additions & 0 deletions R/fit_helpers.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,15 @@
form_form <-
function(object, control, env, ...) {

encoding_info <-
get_encoding(class(object)[1]) %>%
dplyr::filter(mode == object$mode, engine == object$engine)

remove_intercept <- encoding_info %>% dplyr::pull(remove_intercept)
if (remove_intercept) {
env$data <- env$data[, colnames(env$data) != "(Intercept)", drop = FALSE]
}

if (inherits(env$data, "data.frame")) {
check_outcome(eval_tidy(rlang::f_lhs(env$formula), env$data), object)
}
Expand Down