extend to quantile_dist, exclude multi-output #458
Merged
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
|
checks+formatting passed locally, I guess they're not running b/c I'm not targeting dev |
dshemetov
approved these changes
Apr 10, 2025
There was a problem hiding this comment.
Overall looks good, thanks for this! I made a few tweaks
- added and sharpened a few tests after checking code coverage
- fixed some quantile_pred arithmetic edge cases (note: these were needed for tests where we difference quantile pred objects, but might be handy in general)
dsweber2
added a commit
that referenced
this pull request
Apr 15, 2025
* extend to quantile_dist, exclude multi-output * Drop by specification and infer from the epi_df * lint+test: test coverage, handle na lambda case, lint * fix: quantile_pred arithmetic * fix: rlang calls --------- Co-authored-by: Dmitry Shemetov <dshemetov@ucdavis.edu>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Change explanations for reviewer
This grew out of addressing Dan's concerns about the way that outcome handling is done in
layer_Yeo_Johnson. I opted to default tostarts_with(".pred")or whichever columns the user specifies. I dropped support for multiple output columns with different yj_parameters (aheads would be included as different rows rather than columns), and normalize all of them using whichever column corresponds to the output in the recipe'sterm_infoIn addition, I noticed that the inverse yeo_johnson transform wasn't working on
quantile_distobjects. So I rewrote it so that it would, and rewrote the forward transform to match the format.To add a check for that I added
vec_arith.quantile_pred.quantile_pred, though there are ways around it that wouldn't be too nasty.