Skip to content

Conversation

@simonpcouch
Copy link
Contributor

Closes #776.

Worth noting that this error message has changed since the issue was filed, following up on #769.

library(parsnip)

bag_tree %>% set_mode("classification")
#> Error in UseMethod("set_mode"): no applicable method for 'set_mode' applied to an object of class "function"

Created on 2022-08-15 by the reprex package (v2.0.1)

The errors now look like:

library(parsnip)

bag_tree %>% set_mode("classification")
#> Error in `set_mode()`:
#> ! `set_mode()` expected a model specification to be supplied to the
#>   `object` argument, but received a(n) `function` object.
#> ℹ Did you mistakenly pass `model_function` rather than `model_function()`?

bag_tree %>% set_engine("rpart")
#> Error in `set_engine()`:
#> ! `set_engine()` expected a model specification to be supplied to the
#>   `object` argument, but received a(n) `function` object.
#> ℹ Did you mistakenly pass `model_function` rather than `model_function()`?

# won't raise "info" part of error if not a parsnip-namespaced function
# not a function
1L %>% set_args(mode = "classification")
#> Error in `set_args()`:
#> ! `set_args()` expected a model specification to be supplied to the
#>   `object` argument, but received a(n) `integer` object.

# not from parsnip
cos %>% set_mode("classification")
#> Error in `set_mode()`:
#> ! `set_mode()` expected a model specification to be supplied to the
#>   `object` argument, but received a(n) `function` object.

Created on 2022-08-15 by the reprex package (v2.0.1)

Note that, in the case where a user supplies a parsnip-namespaced function that's not a model type function, the error message is misleading. If we wanted to make the logic for that part of the error more exact, we could class the model type functions with a subclass!

@simonpcouch
Copy link
Contributor Author

Requesting your review here, @EmilHvitfeldt, since this messaging is relevant to tidyclust, but no worries on reviewing this before the end of your vacation! These changes won't be merged until after the upcoming release of parsnip anyway, so reviewing in September is great. :)

I use "model specification" in the prompt rather than a specific object class so that this will hopefully generalize well to tidyclust. Feel free to suggest improvements if you think there's a more helpful way of phrasing this, though.

Copy link
Member

@EmilHvitfeldt EmilHvitfeldt left a comment

Choose a reason for hiding this comment

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

Overall good implementation. 1 question in review

@simonpcouch simonpcouch requested a review from topepo August 30, 2022 17:05
@simonpcouch
Copy link
Contributor Author

Looping in @topepo before merging here. See Emil's comment above—does it make more sense to just drop the second bit of the prompt entirely? Or should we think about subclassing the model spec functions so that this logic is solid?

@topepo
Copy link
Member

topepo commented Aug 31, 2022

I agree to leave it as-is (until we see it as a big issue)

@topepo topepo merged commit f7999c0 into main Aug 31, 2022
@topepo topepo deleted the set-error-776 branch August 31, 2022 12:16
@github-actions
Copy link
Contributor

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 Sep 15, 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.

need a better error message for some parsnip functions

4 participants