Skip to content

first pass at postprocessing proof-of-concept #225

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

Merged
merged 31 commits into from
May 31, 2024
Merged

Conversation

simonpcouch
Copy link
Contributor

@simonpcouch simonpcouch commented Apr 23, 2024

Mostly just pattern-matches recipe and model_fit implementations and hooks into the existing post-processing infrastructure. See tests for up-to-date examples.

Previous PR description, outdated

A fast and loose proof-of-concept for integrating a postprocessing container.

library(workflows)
library(parsnip)
library(dplyr)
#> 
#> Attaching package: 'dplyr'
#> The following objects are masked from 'package:stats':
#> 
#>     filter, lag
#> The following objects are masked from 'package:base':
#> 
#>     intersect, setdiff, setequal, union
library(container)
library(modeldata)

table(credit_data$Status)
#> 
#>  bad good 
#> 1254 3200

wflow_class <- fit(workflow(Status ~ ., logistic_reg()), credit_data)

predict(wflow_class, credit_data) %>% table()
#> .pred_class
#>  bad good 
#>  681 3358
 
post <- container(mode = "classification", type = "binary") %>% 
  adjust_probability_threshold(.1)

wflow_class_container <- workflow(Status ~ ., logistic_reg(), post)
wflow_class_container <- fit(wflow_class_container, credit_data)

predict(wflow_class_container, credit_data) %>% table()
#> .pred_class
#>  bad good 
#> 2659 1380

Created on 2024-04-23 with reprex v2.1.0

* move container to Suggests
* document extraction generic
* document `workflow(postprocessor)` argument
@simonpcouch

This comment was marked as resolved.

@simonpcouch

This comment was marked as resolved.

@simonpcouch

This comment was marked as resolved.

R/fit.R Outdated
#' @rdname workflows-internals
#' @export
.fit_post <- function(workflow, data) {
action_post <- workflow[["post"]][["actions"]][["container"]]
Copy link
Member

Choose a reason for hiding this comment

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

use extract_postprocessor() here?

Copy link
Member

@topepo topepo left a comment

Choose a reason for hiding this comment

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

Looks great.

I don't see any issues but I wonder if we should start an issue to make sure that we have the right plumbing and arguments for when we get around to implementing postprocessors for survival models. @hfrick should have a good eye for that here and in general.

@simonpcouch
Copy link
Contributor Author

ec0effa surfaces an important point; removing/updating a postprocessor from an otherwise trained workflow need not remove the preprocessor and model fits, as they won't be affected by the removal of the postprocessor. This introduces the possibility of a "partially trained" workflow, where a workflow with trained preprocessor and model but untrained postprocessor should be able to fit without issue.

to align with the analogous argument in `extract_recipe()`; extract the fitted object if it's available, otherwise extract the specification.

also updates the slot name from `post` to `fit` to elicit that the slot contains the trained object
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.

Okay so here are my thoughts on what plumbing post-processing for survival analysis would need.

Basic assumptions

  • We might tailor both predictions of survival time and predictions of survival probability.
  • If we tailor survival probability, we will do so at a specific single time point (hello eval_time our old friend).

I have not yet validated either assumption. Max, do you have a sense of whether they are valid?

For the implementation, this implies

  • How/where do we specify the predictions?
  • How/where do we specify eval_time?

Specifying the predictions

  • tailor() could take survival time .pred_time via the estimate argument -- the documentation already suggest that.
  • tailor() could take survival probability in the tibble form of .pred (containing .eval_time and .pred_survival) via the probabilities argument.

Specifying eval_time

  • [tailor] tailor() would drop time (the documentation for that as "the predicted event time" contradicts the documentation for estimate listing .pred_time) and instead take eval_time. The alternative would be to derive it from .pred within a given postprocessing operation and default to the first value if we need a single one. Either way, time would disappear.
  • [workflows] We currently can pass eval_time to predict() and augment() methods for workflows but since we currently don't need it earlier, there are not arguments for eval time to the fit() method for workflows or the specification via workflow().
    • If the post-processing operation doesn't need estimation, only predict.workflow() having an eval_time argument should be fine.
    • If it does need estimation, it could come from the tailor() specification.
  • [tune] The tuning functions have an eval_time argument which is required for dynamic and integrated survival metrics. If we need a single eval time to optimize for, we use the first one.
    • If a user includes a tailor in the workflow to be tuned, it would be nice to not make them specify eval_time twice: when making the tailor() and when calling the tuning function.
    • We could pass it through workflows (and make it gain an argument) or make a function to update the tailor. I lean towards updating the tailor.

Comment on lines +121 to +125
check_conflicts.action_tailor <- function(action, x, ..., call = caller_env()) {
post <- x$post

invisible(action)
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this function? We are not doing anything with post so we could just fall back to the default method. We also don't envision different types of post-processors, analogous to how we have different preprocessors (recipe, formula, ...).

Comment on lines +92 to +96
estimate = tidyselect::any_of(c(".pred", ".pred_class")),
probabilities = c(
tidyselect::contains(".pred_"),
-tidyselect::matches("^\\.pred$|^\\.pred_class$")
)
Copy link
Member

Choose a reason for hiding this comment

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

This would need changing if we pass predictions of survival time and survival probability to these arguments. Main catch: the probabilities are in .pred. (The survival times are in .pred_time.)

@hfrick
Copy link
Member

hfrick commented May 8, 2024

After chatting with Max:

  • Max agrees with the basic assumptions except for the single eval time point. He thinks we might want to calibrate/post-process at multiple time points. I agree we might want to do that in general, just not sure about whether or not that is specified/done in one calibration operation (if it requires multiple calibration models to be fitted). Either way, it doesn't change where we need eval time values, just how many. How many is a decision we can make later on.

  • In light of Simon's and my thoughts on specifying the information for the data split needed to fit a workflow with a post-processor that needs fitting on a separate dataset, I've considered adding eval_time there (in add_tailor()) but I think specifying eval_time in tailor() directly is still the right move: it's needed for fitting the post-processor so can't only be in a workflow.

  • Max and I agree we should have an idea of what infrastructure we'd need for post-processing for survival but not include any placeholder arguments at this point. Hence We can remove the time argument in tailors tailor#16

@simonpcouch
Copy link
Contributor Author

With an eye for reducing Remotes hoopla, I'm going to go ahead and merge and open issues for smaller todos.

@simonpcouch simonpcouch marked this pull request as ready for review May 31, 2024 14:20
@simonpcouch simonpcouch merged commit 05be14a into main May 31, 2024
9 checks passed
@simonpcouch simonpcouch deleted the postprocessing branch May 31, 2024 14:23
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 Jun 15, 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.

3 participants