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

adapt test on interaction w/ parsnip re intercept #214

Merged
merged 2 commits into from
Dec 13, 2023

Conversation

hfrick
Copy link
Member

@hfrick hfrick commented Dec 13, 2023

This one is one with a larger backstory ✍️

But maybe the best way in is #210 - the crux here is that workflows added an intercept column (in the test because of the very explicit blueprint, in the issue based on the default to consult the parsnip encodings for the engine) which got treated as a regular predictor by the dot expansion in the model formula. This should not happen, this is what the issue captures.

parsnip's functionality for dealing with that intercept column removed it but because it had entered the terms for the lm,predict.lm() expected it to be there and errored. That is what this test used to capture.

Now parsnip removes the intercept before it enters the terms (tidymodels/parsnip#1033), hence predict.lm() does not fail anymore.

I've adapted the test to try to reflect that. Happy to tweak it if it doesn't come across sufficiently. Alternatively, we could also remove the test since it's covered in extratests also.

It does add a dependency on dev parsnip. It was already in the Remote: section of the description but maybe that was leftover since the version requirement for parsnip was for prior the 1.1.1 version. So maybe that dependency on the dev version is new?

Copy link
Contributor

@simonpcouch simonpcouch left a comment

Choose a reason for hiding this comment

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

These changes look good to me—thanks for the context.🐛

It does add a dependency on dev parsnip.

All is well. I don't believe workflows was part of any of the known "cascades" yet, so shouldn't introduce any conflicts to add that here. :)

Alternatively, we could also remove the test since it's covered in extratests also.

If you think that's a better move, my "Approve" also applies to an adaptation of this PR that just removes this test. You're welcome to merge whenever you please!

tests/testthat/test-predict.R Outdated Show resolved Hide resolved
tests/testthat/test-predict.R Outdated Show resolved Hide resolved
tests/testthat/test-predict.R Outdated Show resolved Hide resolved
tests/testthat/_snaps/pre-action-recipe.md Show resolved Hide resolved
@hfrick hfrick merged commit a73a19d into main Dec 13, 2023
10 checks passed
@hfrick hfrick deleted the workflow-intercept-dot-expansion branch December 13, 2023 15:38
Copy link

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 Dec 28, 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 this pull request may close these issues.

2 participants