-
Notifications
You must be signed in to change notification settings - Fork 93
Closed
Labels
featurea feature request or enhancementa feature request or enhancement
Description
Adding a call to check_args()
on this line https://github.com/tidymodels/parsnip/blob/main/R/misc.R#L350 produces the following results. The only questionable use-case is the last one, which people might use.
Still looking at other places where this would fail
library(parsnip)
library(discrim)
discrim_linear(penalty = -2)
#> Error in `new_model_spec()`:
#> ! The amount of regularization, `penalty`, should be `>= 0`.
discrim_linear(penalty = tune()) %>%
set_args(penalty = 1)
#> Linear Discriminant Model Specification (classification)
#>
#> Main Arguments:
#> penalty = 1
#>
#> Computational engine: MASS
discrim_linear(penalty = tune())
#> Linear Discriminant Model Specification (classification)
#>
#> Main Arguments:
#> penalty = tune()
#>
#> Computational engine: MASS
discrim_linear(penalty = 1) %>%
set_args(penalty = -2)
#> Error in `new_model_spec()`:
#> ! The amount of regularization, `penalty`, should be `>= 0`.
discrim_linear(penalty = -2) %>%
set_args(penalty = 1)
#> Error in `new_model_spec()`:
#> ! The amount of regularization, `penalty`, should be `>= 0`.
Metadata
Metadata
Assignees
Labels
featurea feature request or enhancementa feature request or enhancement
Type
Projects
Milestone
Relationships
Development
Select code repository
Activity
EmilHvitfeldt commentedon Apr 3, 2024
right now the only blocker we have found for this, is dependent on how we handle #1099
simonpcouch commentedon Apr 3, 2024
Very much agreed on the linked issue that we can do a better job checking arguments in
check_args()
.I think it's important to note here that the pattern in
check_args()
methods is toeval_tidy()
each argument:parsnip/R/boost_tree.R
Lines 167 to 169 in dc9a66e
If we're able to
eval_tidy()
each argument to model specification functions as soon as they're inputted, then there's no reason we should be capturing them as quosures in the first place. The reason we capture them as quosures is so that we can evaluate them in an environment that we construct, theeval_env
, atfit()
time:parsnip/R/fit.R
Lines 156 to 158 in dc9a66e
This environment supports defining arguments in relation to the data to be
fit()
ted to, like:...where
.x()
is a data descriptor. As of now,check_args()
knows that it's unable to evaluate such an expression:Created on 2024-04-03 with reprex v2.1.0
Calling
check_args()
will not be able to give the same output when called inside ofnew_model_spec()
because it doesn't yet have access to the data--the fact that the behavior might differ between these two contexts, to me, signals that we ought not tocheck_args()
until we have access to the data atfit()
.There are definitely improvements to be made to argument checking, as in the loophole that you pointed out in the linked issue. I think that's a good place to direct focus for now. :)
simonpcouch commentedon Sep 9, 2024
Given the implications for data descriptors, I'm going to go ahead and close. Still very much on board for #1095. :)
github-actions commentedon Sep 25, 2024
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.