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

code style in checking internals #778

Merged
merged 15 commits into from
Aug 17, 2022
Merged

code style in checking internals #778

merged 15 commits into from
Aug 17, 2022

Conversation

qiushiyan
Copy link
Contributor

When mode == 'unknown', inform_missing_implementation searches available models for all modes.

Also had to use new_model_spec(check_missing_spec = FALSE) in boost_tree (and perhaps other models that involve extension packages) and new_model_spec(check_missing_spec = TRUE) in set_engine.model_spec(), such that inform doesn't use the default engine

# before: parsnip could not locate an implementation for bag_tree model specifications using the `rpart` engine ... 
boost_tree() %>% 
  set_engine("C5.0")

Current error messages

library(parsnip)

bag_tree() %>%
  set_engine("rpart") %>%
  testthat::capture_message()
#> <message/rlang_message>
#> Message:
#> ℹ parsnip could not locate an implementation for bag_tree model specifications using the `rpart` engine. The following parsnip extension packages implement support for this specification.
#> ℹ censored, baguette
#> ℹ Please install one of them (if needed) and load to continue.

bag_tree() %>% 
  set_mode("censored regression") %>% 
  set_engine("rpart") %>% 
  testthat::capture_message()
#> <message/rlang_message>
#> Message:
#> ℹ parsnip could not locate an implementation for bag_tree censored regression model specifications using the `rpart` engine. Please install `censored` (if needed) and load to continue.

bag_tree() %>% 
  set_engine("C5.0") %>% 
  testthat::capture_message()
#> <message/rlang_message>
#> Message:
#> ℹ parsnip could not locate an implementation for bag_tree model specifications using the `C5.0` engine. Please install `baguette` (if needed) and load to continue.

library(censored)
#> Loading required package: survival

bag_tree() %>% 
  set_mode("censored regression") %>% 
  set_engine("rpart") %>% 
  testthat::capture_message()
#> NULL

library(baguette)
bag_tree() %>% 
  set_engine("C5.0") %>% 
  testthat::capture_message()
#> NULL

Created on 2022-08-01 by the reprex package (v2.0.1)
One remaining problem being if set_mode is called after set_engine then the user specified mode won't be reflected in the error message. If set_mode uses new_model_spec(check_missing_arg = TRUE) as well then we get two sets of messages.

@qiushiyan qiushiyan marked this pull request as draft August 1, 2022 20:25
@simonpcouch simonpcouch self-requested a review August 4, 2022 18:10
@simonpcouch
Copy link
Contributor

Thanks for making this happen, @qiushiyan! Feel free to mark this PR as ready for review when you feel good about it and I'll take a look. I had wondered if there were any conditions that this might happen when I put this together, and it looks like I might have missed one. The argument checking across model spec functions is indeed tricky. :)

@qiushiyan qiushiyan marked this pull request as ready for review August 11, 2022 22:39
@qiushiyan
Copy link
Contributor Author

qiushiyan commented Aug 11, 2022

Now compute inform message at print time in print_model_spec() with davis suggestions 🎉 . I think changes in fit() need more thoughts could be in a separate PR. @simonpcouch could you take a look and take this over?

@simonpcouch
Copy link
Contributor

Awesome, thanks for making this happen @qiushiyan. Will review and move this along next week. :)

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.

Mostly reviewing for myself to point out notable changes.

Lots of helpful syntax consistency changes here, though the diffs are a bit difficult to navigate because of them.

Otherwise, as is, relocates check_missing_spec from new_model_spec to print.model_spec, shortens the missing extension package message, and makes needed adjustments!

From here, as Qiushi mentioned, need to figure out actions that indicate defining a model spec is "finished" so we know we can check the mode vs engine vs class. fit is one of those places.

R/engines.R Outdated Show resolved Hide resolved
R/misc.R Outdated Show resolved Hide resolved
@simonpcouch simonpcouch changed the title Better inform message for missing implementations code style in checking internals Aug 17, 2022
@simonpcouch
Copy link
Contributor

Splitting this PR into two, one for code style edits and one to improve messaging for missing implementations. This one is now the former, so will go ahead and merge!

@simonpcouch simonpcouch merged commit 9e36249 into main Aug 17, 2022
@simonpcouch simonpcouch deleted the extension-error branch August 17, 2022 16:08
@github-actions
Copy link

github-actions bot commented Sep 1, 2022

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 1, 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.

None yet

2 participants