Skip to content

frosting layer that creates point and quantiles for any trainer #231

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

Closed
wants to merge 3 commits into from

Conversation

dsweber2
Copy link
Contributor

@dsweber2 dsweber2 commented Sep 8, 2023

There are a couple of ways of going about this:

  1. When making the frosting, determine which layers to insert; this requires the user to hand the trainer used when defining the frosting part of the workflow
  2. determine at runtime the type of .pred, and dispatch on that. to actually implement, this requires one of
    1. copy-pasta-ing large sections of the slather.layer_quantile_distn, slather.layer_point_from_distn, slather.layer_predictive_distn, and slather.layer_residual_quantiles (I think this is a bad idea)
    2. dispatching to the existing slather.layer functions (seems like it would be messy, and would involve faking having the correct objects in the workflow).
    3. refactoring the slather.layer* functions above so that the core doesn't depend on being in a workflow, and then reuse inside (this seems like probably the correct thing, but also fairly involved. Would definitely want to confirm if this is a good plan before potentially messing up the internals).

At the moment, this just does 1; from a user's perspective, the difference isn't that great, so for now I'm doing the easy to implement one, and will come back to a different one based on feedback.
The current function signature is

layer_point_and_distn <- function(frosting, trainer, ...,
                                  probs = c(0.05, 0.95),
                                  symmetrize = TRUE,
                                  by_key = character(0L),
                                  distn_name = ".pred_distn",
                                  point_name = NULL,
                                  distn_id = NULL,
                                  point_id = NULL,
                                  point_type = c("median", "mean"),
                                  truncate = c(-Inf, Inf),
                                  use_predictive_distribution = TRUE,
                                  dist_type = "gaussian")

The arguments are also... maybe a bit much, since they're the arguments of the 4 relevant functions jammed together. This does give the full range of flexibility of the original functions though, and I'm not sure there's a nicer way of guaranteeing that (suggestions welcome).

I'm a bit confused about the difference between probs and levels; it seems like they do the same thing, but have different default values? I put the two together to decrease the plethora of variables above.

@dsweber2 dsweber2 marked this pull request as ready for review September 8, 2023 19:33
@dsweber2 dsweber2 requested a review from dajmcdon as a code owner September 8, 2023 19:33
@dajmcdon
Copy link
Contributor

dajmcdon commented Sep 9, 2023

Meta comment: why add this functionality to this package? I can see how this might help in production, but does this simplify / enhance the experience for the average user? We should add to the agenda for the next tooling meeting.

On probs / levels, see #147

@dsweber2
Copy link
Contributor Author

dsweber2 commented Sep 9, 2023

I would assume the average user isn't going to care that terribly much how their quantiles are produced, and whether they're currently working with quantile_reg or linear_reg. But it's on the pile of topics for the tooling meeting

@dsweber2 dsweber2 force-pushed the bundled_steps_and_layers branch from 786b3dc to 6894071 Compare September 14, 2023 20:51
@dsweber2 dsweber2 force-pushed the bundled_steps_and_layers branch from 6894071 to df116af Compare September 25, 2023 21:33
@dajmcdon dajmcdon closed this Apr 12, 2024
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.

2 participants