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

Glmnet varimp fix 2 #190

Closed
wants to merge 6 commits into from
Closed

Glmnet varimp fix 2 #190

wants to merge 6 commits into from

Conversation

@strongh
Copy link

@strongh strongh commented Jul 21, 2015

See previous thread: #173

@strongh
Copy link
Author

@strongh strongh commented Jul 21, 2015

The Travis error also occurs on master.

@zachmayer
Copy link
Collaborator

@zachmayer zachmayer commented Jul 21, 2015

Yup. @topepo it looks like our builds are failing on master. The relevant errors:

checking dependencies in R code ... WARNING
'::' or ':::' import not declared from: ‘rARPACK’
* checking tests ...
  Running ‘testthat.R’
 ERROR
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
         warning = function(c) invokeRestart("muffleWarning"))
  2: eval(code, new_test_environment)
  3: eval(expr, envir, enclos)
  4: caret:::parse_sampling(i) at test_sampling_options.R:14
  5: checkInstall(pkgs)
  6: stop()

  testthat results ================================================================

The warning is solved with a simple addition of rARPACK to imports. The error looks like a test failure introduced in "added package checks for resampling"

The error occurs in the test_sampling_options.R test.

@topepo
Copy link
Owner

@topepo topepo commented Nov 16, 2015

@strongh Sorry for taking so long to merge this in. Can you sync your PR and resolve any conflicts?

@strongh strongh force-pushed the strongh:glmnet-varimp-fix-2 branch from d94ffd2 to 0d4c677 Nov 17, 2015
@strongh
Copy link
Author

@strongh strongh commented Nov 17, 2015

@topepo looks like it worked.

@topepo
Copy link
Owner

@topepo topepo commented Jan 4, 2016

Can you remove theRData file from the PR?

@strongh
Copy link
Author

@strongh strongh commented Jan 4, 2016

@topepo it has been made so.

topepo added a commit that referenced this pull request Jan 5, 2016
@topepo
Copy link
Owner

@topepo topepo commented Jan 5, 2016

git is fighting me on merging this. I just added the files and committed (see note above) and added your contribution to the NEWS file. You can delete the PR.

Thanks for the fix!

@strongh
Copy link
Author

@strongh strongh commented Jan 5, 2016

👍

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.