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

Add warning message when scaling steps return NaN #1252

Merged
merged 9 commits into from
Nov 16, 2023

Conversation

mastoffel
Copy link
Contributor

@mastoffel mastoffel commented Nov 1, 2023

Adds warning when step_scale() returns NaN columns due to Inf or na_rm=False

Closes #1221

add scaling steps warning test

change test and warning text
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.

Hello @mastoffel 👋 thanks for taking on this work!!

Good work so far!

There are a few other places where we need warnings such as in step_range()

library(recipes)

recipe(~., data = data.frame(x = rep(1, 10))) |>
  step_range(x) |>
  prep() |>
  bake(NULL)
#> # A tibble: 10 × 1
#>        x
#>    <dbl>
#>  1   NaN
#>  2   NaN
#>  3   NaN
#>  4   NaN
#>  5   NaN
#>  6   NaN
#>  7   NaN
#>  8   NaN
#>  9   NaN
#> 10   NaN

or in step_center() if Inf is passed in

library(recipes)

recipe(~., data = data.frame(x = rep(0, 10))) |>
  step_log(x) |>
  step_center(x) |>
  prep() |>
  bake(NULL)
#> # A tibble: 10 × 1
#>        x
#>    <dbl>
#>  1   NaN
#>  2   NaN
#>  3   NaN
#>  4   NaN
#>  5   NaN
#>  6   NaN
#>  7   NaN
#>  8   NaN
#>  9   NaN
#> 10   NaN

If you are not up for those other checks we can change it so it references the issues instead of closing it.

also, please add a bullet in the NEWS file as well. giving yourself credit

R/normalize.R Outdated Show resolved Hide resolved
tests/testthat/test-scale.R Outdated Show resolved Hide resolved
tests/testthat/test-scale.R Outdated Show resolved Hide resolved
@mastoffel
Copy link
Contributor Author

Thanks a lot for the detailed comments @EmilHvitfeldt ! I've added warnings to step_center() and step_range() too now. Let me know if anything is off!

@EmilHvitfeldt
Copy link
Member

Thanks for all the help @mastoffel !! if there are any other issues you want to work on, message in them and we will see what we can do!

@EmilHvitfeldt EmilHvitfeldt merged commit 6bc9cb0 into tidymodels:main Nov 16, 2023
9 checks passed
Copy link

github-actions bot commented Dec 1, 2023

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 Dec 1, 2023
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.

scaling steps should warn if NaN is returned
2 participants