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

Quantile predictions output constructor #1191

Merged
merged 33 commits into from
Sep 13, 2024

Conversation

dajmcdon
Copy link
Contributor

@dajmcdon dajmcdon commented Sep 9, 2024

@topepo

  1. This is just the vctrs constructor.
  2. It only allows a matrix input (with columns corresponding to quantile_levels).
  3. It works with both types of rq() predictions (1 or more tau/quantile_levels).

I added a test for the constructor and adjusted your tests for the quantreg engine.

Don't hesitate to ask for more changes! Or we can revisit if you'd really prefer the tibble list-col version.

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.

Stoked that this is coming together. Will defer to others on feedback related to the design/interface, but some engineering and code-style edits!

R/aaa_quantiles.R Outdated Show resolved Hide resolved
R/aaa_quantiles.R Outdated Show resolved Hide resolved
tests/testthat/test-vec_quantiles.R Outdated Show resolved Hide resolved
tests/testthat/test-vec_quantiles.R Outdated Show resolved Hide resolved
R/aaa_quantiles.R Outdated Show resolved Hide resolved
R/aaa_quantiles.R Outdated Show resolved Hide resolved
@dajmcdon
Copy link
Contributor Author

dajmcdon commented Sep 9, 2024

@simonpcouch Thanks!

Copy link
Member

@EmilHvitfeldt EmilHvitfeldt 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 so far. left a couple of suggestions

R/aaa_quantiles.R Outdated Show resolved Hide resolved
R/aaa_quantiles.R Outdated Show resolved Hide resolved
@topepo
Copy link
Member

topepo commented Sep 10, 2024

Can we call it "quantile_pred"? We have a special "class_pred" vctrs class in probably.

R/aaa_quantiles.R Show resolved Hide resolved
R/aaa_quantiles.R Outdated Show resolved Hide resolved
R/aaa_quantiles.R Show resolved Hide resolved
R/aaa_quantiles.R Outdated Show resolved Hide resolved
R/aaa_quantiles.R Outdated Show resolved Hide resolved
R/aaa_quantiles.R Outdated Show resolved Hide resolved
R/aaa_quantiles.R Outdated Show resolved Hide resolved
R/aaa_quantiles.R Show resolved Hide resolved
@topepo
Copy link
Member

topepo commented Sep 10, 2024

Thanks for making this

Or we can revisit if you'd really prefer the tibble list-col version.

Nope, I'm good with using this class!

@topepo
Copy link
Member

topepo commented Sep 10, 2024

One other small thing... let's remove row and column names from values in parsnip_quantiles(). They carry over into the vctrs object (although you can't see them unless you do something like unclass())

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.

And one more person to chime in 😆 Only small comments though, this is looking great already!

R/aaa_quantiles.R Outdated Show resolved Hide resolved
R/aaa_quantiles.R Outdated Show resolved Hide resolved
R/aaa_quantiles.R Outdated Show resolved Hide resolved
expect_s3_class(one_quant_pred$.pred_quantile[[1]], c("tbl_df", "tbl", "data.frame"))
expect_named(one_quant_pred$.pred_quantile[[1]], c(".pred_quantile", ".quantile_level"))
expect_true(nrow(one_quant_pred$.pred_quantile[[1]]) == 1L)
expect_s3_class(
Copy link
Member

Choose a reason for hiding this comment

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

testthat has expect_vector() specifically for testing vctrs vectors, that seems relevant here.

Copy link
Member

Choose a reason for hiding this comment

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

The output is a vector but the prototype for expect_vector() has matrix and vector inputs whose sizes must match and cannot be zero. It's not very straightforward to do that; It's a quare peg in a round hole. I think that the complexity there is not really worth it.

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.

Woot woot!

R/predict_quantile.R Outdated Show resolved Hide resolved
tests/testthat/test-aaa_quantiles.R Show resolved Hide resolved
R/aaa_quantiles.R Show resolved Hide resolved
@topepo
Copy link
Member

topepo commented Sep 13, 2024

@dajmcdon Merge time!

One note... after looking into performance metrics, we realized that quantile_pred() and its methods will also be needed in yardstick. We want to avoid adding parsnip as a dependency there.

For these cases, we move infrastructure to hardhat (which everyone depends on at some level). After I merge this, I’ll PR into hardhat to put the functions there, and then PR to parsnip to remove them.

@topepo topepo merged commit 3bdb471 into tidymodels:quantile-mode Sep 13, 2024
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 Sep 28, 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.

5 participants