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

implement and document step_intercept #75

Merged
merged 4 commits into from Jul 14, 2017

Conversation

Projects
None yet
3 participants
@alexpghayes
Contributor

alexpghayes commented Jul 13, 2017

Implementation of step_intercept that adds an intercept or constant valued column with role "predictor" re #62 .

Based off of center.R, although I don't know what the @Keywords and @concept tags do so I omitted those in my documentation.

I brought my repo to up even with master and was having some odd troubles with Variable context not set causing vignettes to not build and some tests to fail. Not sure why this was happening. Anyhow, none of the step_intercept code I added sets that off, so I'm assuming we're good.

alexpghayes added some commits Jul 13, 2017

@alexpghayes

This comment has been minimized.

Contributor

alexpghayes commented Jul 14, 2017

i'm not sure what's broken here since my commits were after pulling and bringing my branch to even with recipes/master. more than willing to fix things, but may need a pointer in the right direction. are there any dependencies on dev versions of tidyselect or something like that?

@topepo

This comment has been minimized.

Collaborator

topepo commented Jul 14, 2017

step_intercept creates a specification of a recipe step that will add an intercept or constant term in the first column of a data matrix. step_intercept has predictor role so that it is by default called in the bake step. This means you should always call step_intercept last to avoid unintentional transformations such as centering, scaling, etc.

A better approach is to have another argument that is the name of the new column (maybe default to "intercept"). That would avoid the problem about the sequence of steps. Also, they can exclude the new column using the selectors so you don't have to worry about that.

You can check the reason for the failure by running the tests yourself or look at the "Details" link above. This is failing because:

> rec <- recipe(HHV ~ carbon + hydrogen + oxygen + nitrogen + sulfur,
+               data = biomass_tr)
> rec_trans <- recipe(HHV ~ ., data = biomass_tr[, -(1:2)])
> step_intercept(value = 2)
Error in rec$steps[[length(rec$steps) + 1]] <- object : 
  argument "recipe" is missing, with no default
Calls: step_intercept -> add_step

The first argument of step_intercept(value = 2) should be a recipe so using something like:

 rec_trans <- recipe(HHV ~ ., data = biomass_tr[, -(1:2)]) %>%
   step_intercept(value = 2)

would work.

@codecov-io

This comment has been minimized.

codecov-io commented Jul 14, 2017

Codecov Report

Merging #75 into master will increase coverage by 0.22%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #75      +/-   ##
==========================================
+ Coverage   91.47%   91.69%   +0.22%     
==========================================
  Files          43       44       +1     
  Lines        3107     3132      +25     
==========================================
+ Hits         2842     2872      +30     
+ Misses        265      260       -5
Impacted Files Coverage Δ
R/intercept.R 100% <100%> (ø)
R/recipe.R 75.86% <0%> (+2.46%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d727d9...3a8d695. Read the comment docs.

@alexpghayes

This comment has been minimized.

Contributor

alexpghayes commented Jul 14, 2017

Ah... forgot to compile the documentation changes. Also added a name argument to specify column name.

@topepo

This comment has been minimized.

Collaborator

topepo commented Jul 14, 2017

Looks good. Thanks

@topepo topepo merged commit d965106 into tidymodels:master Jul 14, 2017

3 checks passed

codecov/patch 100% of diff hit (target 91.47%)
Details
codecov/project 91.69% (+0.22%) compared to 7d727d9
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment