Glmnet varimp fix

[strongh]

Currently glmnet's varImp returns both positive and negative values. This is inconsistent with the other implementations, and leads to misleading results when scaled. This PR takes absolute values and also fixes a minor typo that I noticed the same file.

[zachmayer]

Seems good to me. For bonus points, add a testthat test that checks that the glmnet variable importances are positive, even if some coefficients are negative.

[strongh]

I can do that but I may not be able to finish that for a couple days.

@zachmayer could you clarify how I should run these tests?

I've found https://github.com/topepo/caret/blob/593fff8f27034eec290b311c63c01cb6cd763d22/RegressionTests/Code/glmnet.R

and tried running test_file on it, but that hangs. I've also found

https://github.com/topepo/caret/tree/593fff8f27034eec290b311c63c01cb6cd763d22/release_process

but running Rscript RegressionTests/move_files.R doesn't seem to test anything (even after I fixed the obvious hardcoded path issues in that script.

[zachmayer]

For unit tests we use testthat. Add a new file to pkg/caret/tests/testthat that starts with "test" (e.g. test-varImp.R).

Here's an example test. The basic format is:

context("Name of a test suite")
test_that("Name of unit test 1", {
  data(iris)
  expect_is(iris, 'data.frame')
})
test_that("Name of unit test 2", {
  data(iris)
  expect_equal(dim(iris), c(150, 5))
})

[strongh]

Hmm. Travis says my new test fails. After I install my branch, the tests pass for me locally.

@zachmayer any clues on how to reproduce travis' tests? or maybe it's an issue related to packaging the model... in my latest commit I updated models.RData but maybe there's something else that needs to happen?

[zachmayer]

For tests that run on cran, you need to add the libraries required for testing to "Suggests" in the package DESCRIPTION (in this case glmnet would be added to Suggests).

However, since you test is skipped on cran, you can just install the package with travis. Add this line to the install: section of .travis.yml:

  - ./travis-tool.sh r_install glmnet

This will tell travis to install the glmnet package before testing so the test can use that package.

[topepo]

I'm really trying to avoid new packages to the DESCRIPTION files, so please
use the travis fix.

On Fri, Jun 26, 2015 at 12:00 PM, Zach Mayer notifications@github.com
wrote:

For tests that run on cran, you need to add the libraries required for
testing to "Suggests" in the package DESCRIPTION (in this case glmnet would
be added to Suggests).

However, since you test is skipped on cran, you can just install the
package with travis. Add this line to .travis.yml
https://github.com/topepo/caret/blob/master/.travis.yml:

• ./travis-tool.sh r_install glmnet

This will tell travis to install the glmnet package before testing so the
test can use that package.

—
Reply to this email directly or view it on GitHub
#173 (comment).

[zachmayer]

Agreed.

[strongh]

Ok. I've pushed the travis fix but it does not seem to have been effective.

[zachmayer]

Hmm, I'll take a look

[strongh]

It looks like glmnet was installed successfully, but for some reason it's not being detected when the test runs. The package is being installed into /usr/local/lib/R/site-library , which is default.

[zachmayer]

Try adding library(glmnet) to the test? I'm going to try it out locally too.

[zachmayer]

darn, that didn't work.

[zachmayer]

Try adding skip_if_not_installed('glmnet'). Are there any other packages needed for this test that tavis might be missing?

[strongh]

I don't think so.

[zachmayer]

ok, interesting. library(glmnet) is actually failing now. Try library(glmnet, verbose=TRUE).

[strongh]

I had taken out the library(glmnet) when I added the skip_if_not_installed('glmnet'). It doesn't make sense to have them both (right?). So anyway I put the library(glmnet, verbose=TRUE) in, before the skip. So that we can see the output of library(glmnet, verbose=TRUE).

[strongh]

Bump. @zachmayer

Currently the test is just skipped.

[zachmayer]

@strongh Can you rebase against master so we can merge this? Thanks.

[zachmayer]

You also need to resolve merge conflicts in the .travis.yml file.

[strongh]

Ugh. maybe i'll just make a new PR.

[zachmayer]

It'd be good for you to learn how to resolve merge conflicts, but it's not a big deal. Just close this PR and make a new one off the master branch.

Thank you!