Skip to content
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

limit the range of numeric predictions #142

Merged
merged 10 commits into from
Apr 4, 2024
Merged

limit the range of numeric predictions #142

merged 10 commits into from
Apr 4, 2024

Conversation

topepo
Copy link
Member

@topepo topepo commented Apr 4, 2024

A simple post-processor that can cap the range of predicted values form regression models.

@topepo topepo marked this pull request as ready for review April 4, 2024 16:59
@topepo topepo requested a review from simonpcouch April 4, 2024 16:59
Copy link
Contributor

@simonpcouch simonpcouch left a comment

Choose a reason for hiding this comment

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

Looking good😎

In general, I think the arguments to this function would be well-served by the rlang type checkers. There are a good few edge cases (>1-length numeric to upper_limit, non-data-frame x) that would be caught by those utilities, and their messages are quite informative.

R/bound_prediction.R Outdated Show resolved Hide resolved
R/bound_prediction.R Outdated Show resolved Hide resolved
R/bound_prediction.R Outdated Show resolved Hide resolved
tests/testthat/test-bound-prediction.R Outdated Show resolved Hide resolved
tests/testthat/test-bound-prediction.R Outdated Show resolved Hide resolved
library(dplyr)
library(rlang)
data("solubility_test", package = "modeldata")
tune2 <- function() call("tune", "test")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this function ever going to have to deal with tune() values. I guess I'm using my mental model from parsnip here, where upstream packages take care of tune() calls before we send specs off to fit().

Copy link
Member Author

Choose a reason for hiding this comment

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

It might erroneously receive that from users. I'd err on the side of over-checking

Copy link
Member Author

Choose a reason for hiding this comment

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

I added an error for these values (instead of just passing)

R/bound_prediction.R Outdated Show resolved Hide resolved
tests/testthat/Rplots.pdf Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
topepo and others added 2 commits April 4, 2024 14:15
Co-authored-by: Simon P. Couch <simonpatrickcouch@gmail.com>
@topepo
Copy link
Member Author

topepo commented Apr 4, 2024

I think the arguments to this function would be well-served by the rlang type checkers.

Just added them. This would be a good set of issues for TDD 🥳

topepo and others added 4 commits April 4, 2024 14:35
@topepo topepo merged commit 5aa7ecd into main Apr 4, 2024
8 checks passed
@topepo topepo deleted the bound-prediction branch April 4, 2024 19:49
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.

None yet

2 participants