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

step_harmonic() should create predictors #822

Closed
juliasilge opened this issue Sep 27, 2021 · 5 comments
Closed

step_harmonic() should create predictors #822

juliasilge opened this issue Sep 27, 2021 · 5 comments
Labels
bug an unexpected problem or unintended behavior

Comments

@juliasilge
Copy link
Member

The new step_harmonic() should not have NA here because it creates new columns:

role = NA,

Even if I do manually set it to "predictor" something is going wrong with the printing:

library(recipes)
#> Loading required package: dplyr
#> 
#> Attaching package: 'dplyr'
#> The following objects are masked from 'package:stats':
#> 
#>     filter, lag
#> The following objects are masked from 'package:base':
#> 
#>     intersect, setdiff, setequal, union
#> 
#> Attaching package: 'recipes'
#> The following object is masked from 'package:stats':
#> 
#>     step
data(sunspot.year)
sunspots <-
  tibble(year = 1700:1988,
         n_sunspot = sunspot.year)

rec <- 
  recipe(n_sunspot ~ year, data = sunspots) %>%
  step_harmonic(year, frequency = 1 / 11, cycle_size = 1, 
                role = "predictor") 

prep(rec)
#> Recipe
#> 
#> Inputs:
#> 
#>       role #variables
#>    outcome          1
#>  predictor          1
#> 
#> Training data contained 289 data points and no missing data.
#> 
#> Operations:
#> 
#> Harmonic numeric variables for
#> Error in if (nchar(txt) == 0) txt <- "<none>": argument is of length zero

Created on 2021-09-27 by the reprex package (v2.0.1)

@juliasilge juliasilge added the bug an unexpected problem or unintended behavior label Sep 27, 2021
@juliasilge
Copy link
Member Author

I would think this should default to keep_original_cols = FALSE too, right?

@jkennel
Copy link
Contributor

jkennel commented Oct 28, 2021

I can create a pull request for this if you like.

There is an bug in print.step_harmonic.

I can also change the defaults to keep_original_cols = FALSE and role = "predictor".

@juliasilge
Copy link
Member Author

@jkennel That would be fantastic! I believe you have contributed before but if you haven't seen this guidance lately, maybe take a glance.

juliasilge added a commit that referenced this issue Nov 1, 2021
* fix step_harmonic printing bug

* Change default in docs as well

* Update tests, including snaps

Co-authored-by: Julia Silge <julia.silge@gmail.com>
@juliasilge
Copy link
Member Author

Closed in #844

@github-actions
Copy link

This issue 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 Nov 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug an unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

2 participants