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 `rangeBounds` parameter to preProcess() #730

Merged
merged 1 commit into from Oct 24, 2017

Conversation

@sergeykorop
Copy link
Contributor

@sergeykorop sergeykorop commented Sep 2, 2017

This PR proposes following improvement: by adding new parameter, rangeBounds to preProcess() we will be able to specify an explicit interval for range transformation, e.g. [-0.5, 0.5] instead of [0, 1] used by default.

Add an ability to specify interval for `range` transformation.
@codecov-io
Copy link

@codecov-io codecov-io commented Sep 2, 2017

Codecov Report

Merging #730 into master will increase coverage by 0.19%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #730      +/-   ##
==========================================
+ Coverage   16.97%   17.16%   +0.19%     
==========================================
  Files          90       90              
  Lines       13185    13196      +11     
==========================================
+ Hits         2238     2265      +27     
+ Misses      10947    10931      -16
Impacted Files Coverage Δ
R/preProcess.R 38.34% <0%> (+3.19%) ⬆️

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 57b2268...bdd4e2a. Read the comment docs.

@sergeykorop
Copy link
Contributor Author

@sergeykorop sergeykorop commented Sep 2, 2017

This PR interferes with issue #729: unit tests for range transformation of preProcess() are in preProcess_methods.R which is not active for now.

To work-around this issue, I reproduced existing tests related to range in new test file, test_preProcess_range.R and added some new ones covering new functionality. If issue #729 will be settled with bringing preProcess_methods.R back to a pool of active tests, these new tests can be re-integrated back to that file.

@topepo
Copy link
Owner

@topepo topepo commented Oct 24, 2017

Thanks for adding this. You could also get this effect by using the recipes interface to train (just as an fyi)

@topepo topepo merged commit 288f85b into topepo:master Oct 24, 2017
3 checks passed
3 checks passed
codecov/patch Coverage not affected when comparing 57b2268...bdd4e2a
Details
codecov/project 17.16% (+0.19%) compared to 57b2268
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sergeykorop sergeykorop deleted the sergeykorop:rangeBounds branch Oct 25, 2017
sergeykorop added a commit to sergeykorop/caret that referenced this pull request Nov 11, 2017
Separate unit tests for preProcess/range method added in PR topepo#730
are no longer needed after fixing issue topepo#729. Their functionality
has been moved to generic preProcess/methods suite.
topepo added a commit that referenced this pull request Nov 19, 2017
Remove duplicate unit tests introduced in PR #730
@topepo
Copy link
Owner

@topepo topepo commented Nov 21, 2017

@sergeykorop Based on the reverse dependency checks, can you make some quick changes to ensure that the new version of range scaling works with objects from previous versions where those objects don't exist?

For example, the printing is off:

> standardObj
Created from 214 samples and 2343 variables

Pre-processing:
  - ignored (0)
  - re-scaling to [, ] (2343)

Also, for old objects, these lines fail:

newdata[, object$method$range] <- sweep(newdata[, object$method$range, 
    drop = FALSE], 2, (object$ranges[2, ] - object$ranges[1, 
    ])/(object$rangeBounds[2] - object$rangeBounds[1]), "/")
newdata[, object$method$range] <- sweep(newdata[, object$method$range, 
    drop = FALSE], 2, object$rangeBounds[1], "+")
@sergeykorop
Copy link
Contributor Author

@sergeykorop sergeykorop commented Nov 22, 2017

@topepo See PR #790 for bug fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.