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

Refactor choose_metric #777

Merged
merged 11 commits into from
Dec 5, 2023
Merged

Refactor choose_metric #777

merged 11 commits into from
Dec 5, 2023

Conversation

topepo
Copy link
Member

@topepo topepo commented Dec 4, 2023

Uses new code from #768. This only changes the metric code; evaluation times will be the next PR.

Generally, the error messages are improved:

old: (error) "Please check the value of metric.
new (warn): "'WAT' was not in the metric set. Please choose from: 'rmse', 'rsq'"

old (error) "Please specify a single character value for metric."
new (warn) "2 metrics were given; 'rmse' will be used"

This PR also gets rid of the old checks from the second version of tune: "The maximize argument is no longer needed."

Updated PR sequence:

  1. Tools for selecting a default evaluation time #768
  2. (you are here) functions to validate the chosen metric with updates to show_best() and select_best().
  3. functions to validate the chosen evaluation time with updates to show_best() and select_best() (see When we need to default to a single value for eval_time #766).
  4. update finetune to use the change
  5. add additional functions that return (potentially) multiple evaluation times (eg autoplot())
  6. refactor metric input checking code in the tune_* functions, fit_resamples(). etc.
  7. write an article on tidymodels.org about this.

@topepo topepo marked this pull request as ready for review December 4, 2023 18:58
@topepo topepo requested a review from hfrick December 4, 2023 18:58
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.

Looks good! I've added some suggestions on error handling and cli styling since we're already at it.

tests/testthat/test-metric-single-selection.R Outdated Show resolved Hide resolved
tests/testthat/test-select_best.R Show resolved Hide resolved
tests/testthat/test-metric-single-selection.R Show resolved Hide resolved
R/metric-selection.R Outdated Show resolved Hide resolved
R/metric-selection.R Outdated Show resolved Hide resolved
R/metric-selection.R Outdated Show resolved Hide resolved
R/metric-selection.R Outdated Show resolved Hide resolved
R/metric-selection.R Outdated Show resolved Hide resolved
R/metric-selection.R Outdated Show resolved Hide resolved
R/metric-selection.R Outdated Show resolved Hide resolved
@topepo topepo merged commit 1b67e42 into main Dec 5, 2023
9 checks passed
@topepo topepo deleted the new-metric-selections branch December 5, 2023 14:57
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 Dec 20, 2023
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.

2 participants