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

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 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

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 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
@sergeykorop sergeykorop deleted the rangeBounds branch October 25, 2017 09:12
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 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

@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants