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

More survival augment testing #178

Merged
merged 14 commits into from
Feb 14, 2024
Merged

More survival augment testing #178

merged 14 commits into from
Feb 14, 2024

Conversation

EmilHvitfeldt
Copy link
Member

To close #132

Comment on lines 6 to 7
Warning in `show_best()`:
4 evaluation times are available; the first will be used (i.e. `eval_time = 10`).
Copy link
Member Author

Choose a reason for hiding this comment

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

Does this seem right? Should we modify augment.tune_results() such that this doesn't happen. There are currently no way of avoiding this as augment.tune_results() doesn't take the arguments it need

Copy link
Member

@hfrick hfrick Jan 23, 2024

Choose a reason for hiding this comment

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

That's tidymodels/tune#810

We want augment() to treat eval_time like it treats metric: if you don't specify which parameters you want to use, it will use the first metric and pass that to select_best(). Analogous, it should pick the first eval_time and pass that. Both parts documented, of course.

Copy link
Member Author

Choose a reason for hiding this comment

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

got it! augment.tune_results() doesn't have a metric argument, instead it has a parameters argument. These are now tested with fa2099e (#178)

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.

Added suggested changes for tidymodels/tune#832 as an alternative to tidymodels/tune#825

tests/testthat/test-survival-augment.R Outdated Show resolved Hide resolved
tests/testthat/test-survival-augment.R Outdated Show resolved Hide resolved
@hfrick
Copy link
Member

hfrick commented Feb 14, 2024

Looks like the failures on CI are related to the tune exit handlers rather than anything related to survival analysis.

@hfrick hfrick merged commit 234783d into main Feb 14, 2024
3 of 5 checks passed
@hfrick hfrick deleted the more-survival-augment branch February 14, 2024 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test augment() for tune_results, resample_results, and last_fit objects with survival models
2 participants