From 7b95f2002aaf5f7b8f68568d82d210a52d34edec Mon Sep 17 00:00:00 2001 From: topepo Date: Wed, 13 Nov 2024 14:16:42 -0500 Subject: [PATCH 1/6] Changes for #956 --- R/linear_reg.R | 21 +++++++++++++++++ tests/testthat/_snaps/linear_reg.md | 36 +++++++++++++++++++++++++++++ tests/testthat/test-linear_reg.R | 30 ++++++++++++++++++++++++ 3 files changed, 87 insertions(+) diff --git a/R/linear_reg.R b/R/linear_reg.R index 0b7b636b4..c80f7c14b 100644 --- a/R/linear_reg.R +++ b/R/linear_reg.R @@ -73,6 +73,27 @@ translate.linear_reg <- function(x, engine = x$engine, ...) { # evaluated value for the parameter. x$args$penalty <- rlang::eval_tidy(x$args$penalty) } + + # ------------------------------------------------------------------------------ + # We want to avoid folks passing in a poisson family instead of using + # poisson_reg(). It's hard to detect this. + + is_fam <- names(x$eng_args) == "family" + if (any(is_fam)) { + eng_args <- rlang::eval_tidy(x$eng_args[[which(is_fam)]]) + if (is.function(eng_args)) { + eng_args <- try(eng_args(), silent = TRUE) + } + if (inherits(eng_args, "family")) { + eng_args <- eng_args$family + } + if (eng_args == "poisson") { + cli::cli_abort( + "A Poisson family was requested for {.fn linear_reg}. Please use + {.fn poisson_reg} and the engines in the {.pkg poissonreg} package.", + call = rlang::call2("linear_reg")) + } + } x } diff --git a/tests/testthat/_snaps/linear_reg.md b/tests/testthat/_snaps/linear_reg.md index f497ce3da..0ed2c9274 100644 --- a/tests/testthat/_snaps/linear_reg.md +++ b/tests/testthat/_snaps/linear_reg.md @@ -139,3 +139,39 @@ Error in `fit()`: ! `penalty` must be a number larger than or equal to 0 or `NULL`, not the number -1. +# Poisson family (#956) + + Code + linear_reg(penalty = 1) %>% set_engine("glmnet", family = poisson) %>% + translate() + Condition + Error in `linear_reg()`: + ! A Poisson family was requested for `linear_reg()`. Please use `poisson_reg()` and the engines in the poissonreg package. + +--- + + Code + linear_reg(penalty = 1) %>% set_engine("glmnet", family = stats::poisson) %>% + translate() + Condition + Error in `linear_reg()`: + ! A Poisson family was requested for `linear_reg()`. Please use `poisson_reg()` and the engines in the poissonreg package. + +--- + + Code + linear_reg(penalty = 1) %>% set_engine("glmnet", family = stats::poisson()) %>% + translate() + Condition + Error in `linear_reg()`: + ! A Poisson family was requested for `linear_reg()`. Please use `poisson_reg()` and the engines in the poissonreg package. + +--- + + Code + linear_reg(penalty = 1) %>% set_engine("glmnet", family = "poisson") %>% + translate() + Condition + Error in `linear_reg()`: + ! A Poisson family was requested for `linear_reg()`. Please use `poisson_reg()` and the engines in the poissonreg package. + diff --git a/tests/testthat/test-linear_reg.R b/tests/testthat/test-linear_reg.R index 62567fc44..36964a66e 100644 --- a/tests/testthat/test-linear_reg.R +++ b/tests/testthat/test-linear_reg.R @@ -358,3 +358,33 @@ test_that("check_args() works", { } ) }) + + +test_that('Poisson family (#956)', { + expect_snapshot( + linear_reg(penalty = 1) %>% + set_engine("glmnet", family = poisson) %>% + translate(), + error = TRUE + ) + expect_snapshot( + linear_reg(penalty = 1) %>% + set_engine("glmnet", family = stats::poisson) %>% + translate(), + error = TRUE + ) + expect_snapshot( + linear_reg(penalty = 1) %>% + set_engine("glmnet", family = stats::poisson()) %>% + translate(), + error = TRUE + ) + expect_snapshot( + linear_reg(penalty = 1) %>% + set_engine("glmnet", family = "poisson") %>% + translate(), + error = TRUE + ) + + +}) From b9f45eb52bd5733b366f7a220b168d676257cfb6 Mon Sep 17 00:00:00 2001 From: topepo Date: Wed, 13 Nov 2024 14:23:12 -0500 Subject: [PATCH 2/6] update news --- NEWS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS.md b/NEWS.md index e35af6bcd..692f5f89a 100644 --- a/NEWS.md +++ b/NEWS.md @@ -23,6 +23,8 @@ * Aligned `null_model()` with other model types; the model type now has an engine argument that defaults to `"parsnip"` and is checked with the same machinery that checks other model types in the package (#1083). +* If linear regression is requested with a Poisson family, an error will occur and refer the user to `poisson_reg()` (#956) + ## Bug Fixes * Make sure that parsnip does not convert ordered factor predictions to be unordered. From 72e01d1b030faf16781f4782ab1e7a6b1693aafe Mon Sep 17 00:00:00 2001 From: Hannah Frick Date: Mon, 9 Dec 2024 16:53:31 +0000 Subject: [PATCH 3/6] point to parsnip PR instead of tune issue --- NEWS.md | 2 +- tests/testthat/_snaps/linear_reg.md | 2 +- tests/testthat/test-linear_reg.R | 4 +--- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/NEWS.md b/NEWS.md index 692f5f89a..23a269c07 100644 --- a/NEWS.md +++ b/NEWS.md @@ -23,7 +23,7 @@ * Aligned `null_model()` with other model types; the model type now has an engine argument that defaults to `"parsnip"` and is checked with the same machinery that checks other model types in the package (#1083). -* If linear regression is requested with a Poisson family, an error will occur and refer the user to `poisson_reg()` (#956) +* If linear regression is requested with a Poisson family, an error will occur and refer the user to `poisson_reg()` (#1219). ## Bug Fixes diff --git a/tests/testthat/_snaps/linear_reg.md b/tests/testthat/_snaps/linear_reg.md index 0ed2c9274..1fc287ac4 100644 --- a/tests/testthat/_snaps/linear_reg.md +++ b/tests/testthat/_snaps/linear_reg.md @@ -139,7 +139,7 @@ Error in `fit()`: ! `penalty` must be a number larger than or equal to 0 or `NULL`, not the number -1. -# Poisson family (#956) +# prevent using a Poisson family Code linear_reg(penalty = 1) %>% set_engine("glmnet", family = poisson) %>% diff --git a/tests/testthat/test-linear_reg.R b/tests/testthat/test-linear_reg.R index 36964a66e..969cb994c 100644 --- a/tests/testthat/test-linear_reg.R +++ b/tests/testthat/test-linear_reg.R @@ -360,7 +360,7 @@ test_that("check_args() works", { }) -test_that('Poisson family (#956)', { +test_that("prevent using a Poisson family", { expect_snapshot( linear_reg(penalty = 1) %>% set_engine("glmnet", family = poisson) %>% @@ -385,6 +385,4 @@ test_that('Poisson family (#956)', { translate(), error = TRUE ) - - }) From d6f2e26525ff48bf33b7b72ee3c981fad11f090d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=98topepo=E2=80=99?= Date: Wed, 29 Jan 2025 08:05:34 -0500 Subject: [PATCH 4/6] move poisson checks to check_args --- R/linear_reg.R | 41 +++++++++++++++-------------- tests/testthat/_snaps/linear_reg.md | 12 ++++----- tests/testthat/test-linear_reg.R | 8 +++--- 3 files changed, 31 insertions(+), 30 deletions(-) diff --git a/R/linear_reg.R b/R/linear_reg.R index c80f7c14b..e0def96fd 100644 --- a/R/linear_reg.R +++ b/R/linear_reg.R @@ -74,26 +74,6 @@ translate.linear_reg <- function(x, engine = x$engine, ...) { x$args$penalty <- rlang::eval_tidy(x$args$penalty) } - # ------------------------------------------------------------------------------ - # We want to avoid folks passing in a poisson family instead of using - # poisson_reg(). It's hard to detect this. - - is_fam <- names(x$eng_args) == "family" - if (any(is_fam)) { - eng_args <- rlang::eval_tidy(x$eng_args[[which(is_fam)]]) - if (is.function(eng_args)) { - eng_args <- try(eng_args(), silent = TRUE) - } - if (inherits(eng_args, "family")) { - eng_args <- eng_args$family - } - if (eng_args == "poisson") { - cli::cli_abort( - "A Poisson family was requested for {.fn linear_reg}. Please use - {.fn poisson_reg} and the engines in the {.pkg poissonreg} package.", - call = rlang::call2("linear_reg")) - } - } x } @@ -134,5 +114,26 @@ check_args.linear_reg <- function(object, call = rlang::caller_env()) { check_number_decimal(args$mixture, min = 0, max = 1, allow_null = TRUE, call = call, arg = "mixture") check_number_decimal(args$penalty, min = 0, allow_null = TRUE, call = call, arg = "penalty") + # ------------------------------------------------------------------------------ + # We want to avoid folks passing in a poisson family instead of using + # poisson_reg(). It's hard to detect this. + + is_fam <- names(object$eng_args) == "family" + if (any(is_fam)) { + eng_args <- rlang::eval_tidy(object$eng_args[[which(is_fam)]]) + if (is.function(eng_args)) { + eng_args <- try(eng_args(), silent = TRUE) + } + if (inherits(eng_args, "family")) { + eng_args <- eng_args$family + } + if (eng_args == "poisson") { + cli::cli_abort( + "A Poisson family was requested for {.fn linear_reg}. Please use + {.fn poisson_reg} and the engines in the {.pkg poissonreg} package.", + call = rlang::call2("linear_reg")) + } + } + invisible(object) } diff --git a/tests/testthat/_snaps/linear_reg.md b/tests/testthat/_snaps/linear_reg.md index 1fc287ac4..fd6acb8bb 100644 --- a/tests/testthat/_snaps/linear_reg.md +++ b/tests/testthat/_snaps/linear_reg.md @@ -142,8 +142,8 @@ # prevent using a Poisson family Code - linear_reg(penalty = 1) %>% set_engine("glmnet", family = poisson) %>% - translate() + linear_reg(penalty = 1) %>% set_engine("glmnet", family = poisson) %>% fit(mpg ~ + ., data = mtcars) Condition Error in `linear_reg()`: ! A Poisson family was requested for `linear_reg()`. Please use `poisson_reg()` and the engines in the poissonreg package. @@ -152,7 +152,7 @@ Code linear_reg(penalty = 1) %>% set_engine("glmnet", family = stats::poisson) %>% - translate() + fit(mpg ~ ., data = mtcars) Condition Error in `linear_reg()`: ! A Poisson family was requested for `linear_reg()`. Please use `poisson_reg()` and the engines in the poissonreg package. @@ -161,7 +161,7 @@ Code linear_reg(penalty = 1) %>% set_engine("glmnet", family = stats::poisson()) %>% - translate() + fit(mpg ~ ., data = mtcars) Condition Error in `linear_reg()`: ! A Poisson family was requested for `linear_reg()`. Please use `poisson_reg()` and the engines in the poissonreg package. @@ -169,8 +169,8 @@ --- Code - linear_reg(penalty = 1) %>% set_engine("glmnet", family = "poisson") %>% - translate() + linear_reg(penalty = 1) %>% set_engine("glmnet", family = "poisson") %>% fit( + mpg ~ ., data = mtcars) Condition Error in `linear_reg()`: ! A Poisson family was requested for `linear_reg()`. Please use `poisson_reg()` and the engines in the poissonreg package. diff --git a/tests/testthat/test-linear_reg.R b/tests/testthat/test-linear_reg.R index 969cb994c..7473ead85 100644 --- a/tests/testthat/test-linear_reg.R +++ b/tests/testthat/test-linear_reg.R @@ -364,25 +364,25 @@ test_that("prevent using a Poisson family", { expect_snapshot( linear_reg(penalty = 1) %>% set_engine("glmnet", family = poisson) %>% - translate(), + fit(mpg ~ ., data = mtcars), error = TRUE ) expect_snapshot( linear_reg(penalty = 1) %>% set_engine("glmnet", family = stats::poisson) %>% - translate(), + fit(mpg ~ ., data = mtcars), error = TRUE ) expect_snapshot( linear_reg(penalty = 1) %>% set_engine("glmnet", family = stats::poisson()) %>% - translate(), + fit(mpg ~ ., data = mtcars), error = TRUE ) expect_snapshot( linear_reg(penalty = 1) %>% set_engine("glmnet", family = "poisson") %>% - translate(), + fit(mpg ~ ., data = mtcars), error = TRUE ) }) From e322109ba61f2f25b66216dab7e788c1c9d4d686 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=98topepo=E2=80=99?= Date: Wed, 29 Jan 2025 12:08:25 -0500 Subject: [PATCH 5/6] the snapshots should be created without glmnet installed --- tests/testthat/_snaps/linear_reg.md | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/testthat/_snaps/linear_reg.md b/tests/testthat/_snaps/linear_reg.md index fd6acb8bb..229828f30 100644 --- a/tests/testthat/_snaps/linear_reg.md +++ b/tests/testthat/_snaps/linear_reg.md @@ -145,8 +145,8 @@ linear_reg(penalty = 1) %>% set_engine("glmnet", family = poisson) %>% fit(mpg ~ ., data = mtcars) Condition - Error in `linear_reg()`: - ! A Poisson family was requested for `linear_reg()`. Please use `poisson_reg()` and the engines in the poissonreg package. + Error in `fit()`: + ! Please install the glmnet package to use this engine. --- @@ -154,8 +154,8 @@ linear_reg(penalty = 1) %>% set_engine("glmnet", family = stats::poisson) %>% fit(mpg ~ ., data = mtcars) Condition - Error in `linear_reg()`: - ! A Poisson family was requested for `linear_reg()`. Please use `poisson_reg()` and the engines in the poissonreg package. + Error in `fit()`: + ! Please install the glmnet package to use this engine. --- @@ -163,8 +163,8 @@ linear_reg(penalty = 1) %>% set_engine("glmnet", family = stats::poisson()) %>% fit(mpg ~ ., data = mtcars) Condition - Error in `linear_reg()`: - ! A Poisson family was requested for `linear_reg()`. Please use `poisson_reg()` and the engines in the poissonreg package. + Error in `fit()`: + ! Please install the glmnet package to use this engine. --- @@ -172,6 +172,6 @@ linear_reg(penalty = 1) %>% set_engine("glmnet", family = "poisson") %>% fit( mpg ~ ., data = mtcars) Condition - Error in `linear_reg()`: - ! A Poisson family was requested for `linear_reg()`. Please use `poisson_reg()` and the engines in the poissonreg package. + Error in `fit()`: + ! Please install the glmnet package to use this engine. From 362e44f7e8d4f0d8dd7c0d7491bab43969a8ebf8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=98topepo=E2=80=99?= Date: Wed, 29 Jan 2025 12:26:52 -0500 Subject: [PATCH 6/6] =?UTF-8?q?GHA=20for=20tests=20has=20glmnet=20installe?= =?UTF-8?q?d=20(=E2=97=94=5F=E2=97=94)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- tests/testthat/test-linear_reg.R | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/testthat/test-linear_reg.R b/tests/testthat/test-linear_reg.R index 7473ead85..ef0022feb 100644 --- a/tests/testthat/test-linear_reg.R +++ b/tests/testthat/test-linear_reg.R @@ -361,6 +361,7 @@ test_that("check_args() works", { test_that("prevent using a Poisson family", { + skip_if(rlang::is_installed("glmnet")) expect_snapshot( linear_reg(penalty = 1) %>% set_engine("glmnet", family = poisson) %>%