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

Tools for selecting a default evaluation time #768

Merged
merged 7 commits into from
Dec 4, 2023

Conversation

topepo
Copy link
Member

@topepo topepo commented Nov 28, 2023

show_best() will pick an evaluation time for a dynamic metric when none is given.

Previously, we would find what was in the data and select a time that was close to the median time. This was fine but inconsistent with other parts of tidymodels that do similar operations. For example, tune_bayes has to have a metric to optimize on so it uses the first metric in the metric set and, if needed, the first evaluation time given to the function.

This PR adds a few exported but internal helper functions (primarily first_eval_time()) as the canonical tools for these selections.

This is one of a sequence of PRs:

  1. you are here
  2. functions to validate the chosen metric and evaluation times
  3. refactor the internals of show_best() to be more modular with these tools
  4. propagate these changes to other functions that either use show_best() or do similar computations (see When we need to default to a single value for eval_time #766)
  5. update finetune to use the change
  6. add additional functions that return (potentially) multiple evaluation times (eg autoplot())
  7. write an article on tidymodels.org about this.

@topepo topepo marked this pull request as ready for review November 28, 2023 22:57
@topepo topepo requested a review from hfrick November 28, 2023 22:57
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.

There are a couple of suggested changes in terms of style for the tests so highlighting here one comment on structure: We also have logic on when to error/warn in terms of combinations of metric type and eval_time in check_eval_time() which we may want to consolidate in the series of PRs here.

tests/testthat/test-eval-time-single-selection.R Outdated Show resolved Hide resolved
tests/testthat/test-eval-time-single-selection.R Outdated Show resolved Hide resolved
tests/testthat/test-eval-time-single-selection.R Outdated Show resolved Hide resolved
tests/testthat/test-eval-time-single-selection.R Outdated Show resolved Hide resolved
tests/testthat/test-eval-time-single-selection.R Outdated Show resolved Hide resolved
tests/testthat/test-eval-time-single-selection.R Outdated Show resolved Hide resolved
tests/testthat/test-eval-time-single-selection.R Outdated Show resolved Hide resolved
R/metric-selection.R Show resolved Hide resolved
R/metric-selection.R Show resolved Hide resolved
R/select_best.R Show resolved Hide resolved
topepo and others added 2 commits December 2, 2023 12:49
Co-authored-by: Hannah Frick <hfrick@users.noreply.github.com>
@topepo
Copy link
Member Author

topepo commented Dec 2, 2023

This will pass CI when #776 is merged :-<

@topepo topepo merged commit 8d71b2c into main Dec 4, 2023
9 checks passed
@topepo topepo deleted the refactor-eval-time-default branch December 4, 2023 17:28
@topepo
Copy link
Member Author

topepo commented Dec 6, 2023

Now I remember why check_select_dots() uses enquos(...) instead of list(...)... the latter evaluates the items and that fails when the dots contain something like desc(K) (like in the examples).

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 21, 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