Skip to content

Conversation

@simonpcouch
Copy link
Contributor

Introduces the missing implementation prompt as an error on extract_parameter*dials. Only needs added to extract_parameter_set_dials, which is called by extract_parameter_dials. We only add this to the .model_spec methods, as the prompt is raised already for workflows via tidymodels/workflows#175 by then. :)

library(parsnip)

bt_mod <- bag_tree(min_n = tune()) %>%
  set_mode("regression")

extract_parameter_set_dials(bt_mod)
#> Error:
#> ! parsnip could not locate an implementation for `bag_tree` regression
#>   model specifications.
#> ℹ The parsnip extension package baguette implements support for this
#>   specification.
#> ℹ Please install (if needed) and load to continue.

extract_parameter_dials(bt_mod, parameter = "min_n")
#> Error:
#> ! parsnip could not locate an implementation for `bag_tree` regression
#>   model specifications.
#> ℹ The parsnip extension package baguette implements support for this
#>   specification.
#> ℹ Please install (if needed) and load to continue.

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

Relies on #804 for a more fully readable when the user has supplied an engine.

@simonpcouch simonpcouch requested a review from hfrick September 9, 2022 13:54
Copy link
Member

@hfrick hfrick left a comment

Choose a reason for hiding this comment

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

Nice!

R/extract.R Outdated
Comment on lines 77 to 83
if (!spec_is_loaded(
cls = class(x)[1],
engine = x$engine,
user_specified_engine = x$user_specified_engine,
mode = x$mode,
user_specified_mode = x$user_specified_mode
)) {
Copy link
Member

Choose a reason for hiding this comment

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

if that call to spec_is_loaded() with args that are all derived from x is a common pattern elsewhere in parsnip, it might be worth putting that in its own little wrapper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I feel this.

Something near it is quite common, though usually one or two of these arguments is switched out with a "proposed" slot (like, in set_mode, mode = mode and user_specified_mode = TRUE). Maybe the first argument ought to be x, with defaults to x$ slots, and we specify when needed?

Copy link
Member

Choose a reason for hiding this comment

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

that sounds like a small, good quality-of-life improvement to me 👍

by passing the spec rather than it's class to the `spec_is_*` functions, we can cut down on the number of arguments we need to supply by setting defaults based on `spec`s slots, only supplying additional arguments when we "propose" new slots in `set_` functions.
@simonpcouch simonpcouch requested a review from hfrick September 12, 2022 21:18
@simonpcouch simonpcouch merged commit c24b014 into main Sep 13, 2022
@simonpcouch simonpcouch deleted the extract-prompt branch September 13, 2022 13:14
@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 28, 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