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

Placement of the eval_time argument #857

Merged
merged 11 commits into from
Feb 28, 2024
Merged

Placement of the eval_time argument #857

merged 11 commits into from
Feb 28, 2024

Conversation

hfrick
Copy link
Member

@hfrick hfrick commented Feb 27, 2024

This PR moves the eval_time argument right after metrics (or metric) for user-facing functions:

  • fit_resamples(), tune_grid(), tune_bayes()
  • last_fit()
  • fit_best()
  • show_best(), select_by_pct_loss()
  • autoplot()

Corresponding changes should happen in finetune for tune_race_anova(), tune_race_win_loss(), and tune_sim_anneal().

This PR does not touch .estimate_metrics() because the eval_time argument there should be removed, see #856. It also does not touch the argument order of the internal loop functions because the inconsistent naming of other arguements paired with passing arguments by position makes it likely to introduce bugs, so leaving this to #742.

The revdeps for these changes:

> revdepcheck::cloud_summary()
ℹ Syncing results to revdep/cloud.noindex/e7d0b88d-96f8-42a4-8b7e-a5ae07e69610
ℹ Comparing results
✔ agua 0.1.3                             ── E: 0     | W: 0     | N: 1    
✔ bonsai 0.2.1                           ── E: 0     | W: 0     | N: 0    
✔ finetune 1.1.0                         ── E: 1     | W: 0     | N: 0    
✔ finnts 0.4.0                           ── E: 0     | W: 0     | N: 0    
✔ healthyR.ai 0.0.13                     ── E: 1     | W: 1     | N: 0    
✔ healthyR.ts 0.3.0                      ── E: 0     | W: 0     | N: 1    
✔ HTRX 1.2.4                             ── E: 0     | W: 0     | N: 1    
✔ modeltime 1.2.8                        ── E: 0     | W: 0     | N: 1    
✔ modeltime.ensemble 1.0.3               ── E: 0     | W: 0     | N: 1    
✔ modeltime.resample 0.2.3               ── E: 0     | W: 0     | N: 1    
✔ nestedmodels 1.1.0                     ── E: 0     | W: 0     | N: 0    
✔ offsetreg 1.0.0                        ── E: 0     | W: 0     | N: 0    
✔ probably 1.0.3                         ── E: 0     | W: 0     | N: 0    
✔ shinymodels 0.1.1                      ── E: 0     | W: 0     | N: 0    
✔ stacks 1.0.3                           ── E: 0     | W: 0     | N: 0    
✔ tabnet 0.5.0                           ── E: 0     | W: 0     | N: 0    
✔ text 1.2.0                             ── E: 0     | W: 0     | N: 1    
✖ tidyclust 0.2.0                        ── E: 0  +1 | W: 0     | N: 0    
✔ tidydann 1.0.0                         ── E: 0     | W: 0     | N: 0    
✔ tidymodels 1.1.1                       ── E: 0     | W: 0     | N: 0    
✔ tidyposterior 1.0.1                    ── E: 1     | W: 0     | N: 0    
✖ tidysdm 0.9.3                          ── E: 0  +2 | W: 0  +1 | N: 0    
✔ timetk 2.9.0                           ── E: 0     | W: 0     | N: 1    
✔ usemodels 0.2.0                        ── E: 0     | W: 0     | N: 0    
✔ viralmodels 1.2.0                      ── E: 0     | W: 0     | N: 0    
✔ viruslearner 0.0.1                     ── E: 0     | W: 0     | N: 0    
✔ workflowsets 1.0.1                     ── E: 0     | W: 0     | N: 0   

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.

On board for mostly not touching grid_code_paths.R. Does check_initial() need a change here?

eval_time = eval_time,
control = control,
add_validation_set = add_validation_set
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding the names here!

Copy link
Member Author

Choose a reason for hiding this comment

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

Safety! 😆

@hfrick
Copy link
Member Author

hfrick commented Feb 27, 2024

Good shout! Added in ff1af86

@hfrick
Copy link
Member Author

hfrick commented Feb 27, 2024

The changes for finetune are in tidymodels/finetune#104

The tests in extratests are being run for both PRs in tidymodels/extratests#201

Without the changes to check_initial() they already ran successfully. The current run includes those changes + the finetune changes.

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.

Looking good. :)

From my perspective, this PR and it's finetune friend are ready to merge!

I'd say we wait to file revdep breakages until we've merged the coming dots changes and possible brier score introduction in #859, as there may be some overlap.

@simonpcouch simonpcouch merged commit a8f0772 into main Feb 28, 2024
9 checks passed
@simonpcouch simonpcouch deleted the eval_time-placement branch February 28, 2024 16:53
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 Mar 14, 2024
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