bug fixes for percentile intervals on survival metrics #818
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
tidymodels/rsample#465 includes a proposal for some rsample changes. I don't want to force an rsample release so we can keep this PR as-is and, if the rsample changes are ok, update this code later. |
hfrick
approved these changes
Jan 22, 2024
| @@ -210,6 +210,7 @@ export(is_recipe) | |||
| export(is_workflow) | |||
| export(last_fit) | |||
| export(load_pkgs) | |||
| export(maybe_choose_eval_time) | |||
There was a problem hiding this comment.
Mostly future-proofing; I can see us needing this a lot inside of tune but also maybe finetune or future extensions. There's no harm in exporting and we've re-exported a fair number of functions that were originally internal.
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The initial implementation was only for purely dynamic metrics.
Changes:
quietlyargument tochoose_eval_time()maybe_choose_eval_time()) that runschoose_eval_time()across the metrics in a metric set to get defaults for all metrics. If an integrated metric is in the set, all of the original evaluation times (from the tune object) are returned.eval_timearguments through the missing bootstrap interval machinery.int_pctl()Tests to come in extratests
note: the percentile intervals are calculated for each value of the
termcolumn (via agroup_by(). It would greatly simplify things here if we expanded that to include a few other columns:.config,.iter, and.eval_time. In fact, it might help users if we were to group bytermandstarts_with("."). I'll start an issue for this in rsample.