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

transition update internals #704

Merged
merged 14 commits into from
May 4, 2022
Merged

transition update internals #704

merged 14 commits into from
May 4, 2022

Conversation

simonpcouch
Copy link
Contributor

See #691 and #691 (comment). :)

no need to snapshot the `print.parameters` method: it may change soon, and some extension packages aren't using 3e yet
this way, we can `Remotes` a specific parsnip version before package releases
@simonpcouch simonpcouch marked this pull request as draft April 21, 2022 17:44
@simonpcouch simonpcouch removed the request for review from juliasilge April 21, 2022 17:44
@simonpcouch
Copy link
Contributor Author

re: follow-up discussion from the group meeting, going to go ahead and work some update refactoring into this PR. :)

A la

parsnip/R/mlp.R

Line 103 in 68dfd60

# TODO make these blocks into a function and document well

The process:

* put together a `update_spec` helper that does most all of the work that the `update` methods do
* for each update method:
     + drop in the `update_spec` template
     + update it’s `cls` argument to match the same `new_model_spec` argument
     + if the `update` method doesn’t take a parameters argument, set it to NULL in `update_spec`
     + delete remaining code besides the `args` `enquo`ting lines

Push before updating any of the tests to make sure that checks pass on the previous tests too.
`fresh` was ignored and engine arguments propagated through `update`
transition most tests to `test_update` and test more thoroughly with one example. for each update method test, pass both a model and engine argument, and then `update` both of them to `tune()` (with some exceptions depending on available arguments).
@simonpcouch
Copy link
Contributor Author

Okay, good to go! This PR now refactors the internals of update methods a bit more thoroughly and consolidates their unit tests.

One thing worth mentioning—I'm not sure if this was intended behavior, but update_engine_parameters used to retain engine args even when fresh = TRUE:

pak::pkg_install("tidymodels/parsnip")
#> ℹ Loading metadata database
#> ℹ Loading metadata database
  
library(parsnip)  
  
linear_reg(mixture = 0) %>% 
  set_engine("glmnet", nlambda = 10) %>% 
  update(penalty = 1, fresh = TRUE)
#> Linear Regression Model Specification (regression)
#> 
#> Main Arguments:
#>   penalty = 1
#> 
#> Engine-Specific Arguments:
#>   nlambda = 10
#> 
#> Computational engine: glmnet

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

It now handles engine args in the same way we've previously handled main ones:

pak::pkg_install("tidymodels/parsnip@engine-params-691")
#> ℹ Loading metadata database
#> ℹ Loading metadata database
  
library(parsnip)  
  
linear_reg(mixture = 0) %>% 
  set_engine("glmnet", nlambda = 10) %>% 
  update(penalty = 1, fresh = TRUE)
#> Linear Regression Model Specification (regression)
#> 
#> Main Arguments:
#>   penalty = 1
#> 
#> Computational engine: glmnet

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

🐝

Copy link
Member

@juliasilge juliasilge left a comment

Choose a reason for hiding this comment

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

This is really an improvement that will help a lot! 🙌 Consolidating those tests is a great move.

@topepo
Copy link
Member

topepo commented Apr 27, 2022

One thing worth mentioning—I'm not sure if this was intended behavior, but update_engine_parameters used to retain engine args even when fresh = TRUE

That was not intended. Can you update this PR with that change?

@simonpcouch
Copy link
Contributor Author

@topepo That change is already made here! Added a bullet to NEWS re: that fix—give me a holler if that's not what you meant.

@simonpcouch
Copy link
Contributor Author

Glad to fix merge conflicts if this PR is otherwise good to go!🐨

@topepo topepo merged commit 083fd9a into main May 4, 2022
@topepo topepo deleted the engine-params-691 branch May 4, 2022 17:17
@github-actions
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 May 19, 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.

3 participants