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.
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
first pass at the post-processing container #1
first pass at the post-processing container #1
Changes from all commits
69f1351
3b4f35f
56ca8e5
c1b173e
c62ac77
38b5662
8f0b4a2
162c212
c07348c
979841c
00efa2a
196a0ca
66f6e2e
2037357
aeca10d
7509ba0
025ae83
319ac5f
4164156
2689bdb
4db51af
6e811da
17e154a
fa1f95d
0eb7a04
561a795
b92edb0
809da4a
a5d5843
661a323
e3038ba
fbc9fbe
cfe9455
9e4f483
72d1007
ab07580
3cab3e0
917196f
5e6f981
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, this function (and it's probability analog) take a trained
calibrator
and then don't do any actual fitting atfit()
time. This means that:truth
andestimate
(and by different names!).truth
andestimate
columns when they didn't used to need to.With the current draft, this looks like:
Created on 2024-04-24 with reprex v2.1.0
I propose that we supply some interface in place of
calibrator
that just specifies the kind of calibration that would happen, e.g.type = "linear", ...
orfn = cal_estimate_linear, ...
and actually fit the calibrator atfit()
time. This would allow for workflows users to never have to specify column names and container users to only specify column names once. This would look something like:Created on 2024-04-24 with reprex v2.1.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is: what data do you have to do that?
In the course of tuning, you only have the holdout predictions for that resample. If we had the whole set of out-of-sample predictions, we could train on that. Using re-predicted training data isn't a good option.
Another idea is to supply a
data
argument where people can pass in any predictions that they want. However, unless they have a specific validation set held out for this specific operation, we are back to creating those predictions outside of the workflow/tune process.I'm not happy with what the process is now but I think it is what we can do (for now, at least).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that I'll write a blog post about this to outline my thoughts better (and we can refer to that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current approach leans on users to figure that out--an approach like the one I've proposed allows us to take care of that for the user.
I think framing this in the context of tuning/resampling, like you've done, is helpful. When I think through the interface as implemented, I'm unsure how to imagine tuning parameters from the container.
tune_*()
takes in one workflow and resamples it. In the current approach, how is that workflow's postprocessor trained? The calibrator needs predictions, and based on your reply, we don't want those predictions to come from data that the model was trained on. So, do we take a three-way split, train/test the validation split, train some model on the training portion of the validation split, train the calibrator on the testing portion of the validation split, and then pass that fixed calibrator to the workflow? When tuning, which of the resampled models is the one used to train the calibrator? e.g., as I understand it, this would be the resampling flow as implemented:If we just allow specifying the strategy used to train a post-processor rather than the trained post-processor, the post-processor is actually resampled and the user interface feels much more like tidymodels. Also, the onus of doing the right thing w.r.t. resampling is on us rather than the user:
From my understanding of our conversation earlier in the week, you've observed that training calibrators on re-predicted data is typically not a problem practically.
That said, from a technical standpoint, this feels quite solvable in tune: