Allowing binomial option

[jlstiles]

This now allows the binomial option and for some reason I needed family$family within hal function or it would give me an error about the arg not being NULL or a character.

[jlstiles]

I should say, it does work fine with family specified as binomial() or gaussian() as normally declared in either SL.hal or hal.

[nhejazi]

Based on the current Travis CI results, it appears you need to rebuild the package documentation (you can do this by running devtools::document()). After this, add the updated docs as a new commit to this PR. Feel free to ignore the Appveyor CI results when considering merging this PR (the Windows build appears to be broken on branch master anyway).

[jlstiles]

I rebuilt the docs but still failing Travis. I don't understand these tests or why the test is saying args are not matching in error logs.

[ck37]

Looks like the issue is that the new family argument to hal() doesn't have a default, but it's a new option so it will cause any previously created hal() test to fail because it wouldn't have specified the family option. See this code: https://github.com/benkeser/halplus/blob/master/tests/testthat/test_basic.R#L18-L23

So you could specify a default of family = gaussian() in hal() for backwards compatibility, or be ok with breaking old code but at least update the test(s) to specify the family.

[jlstiles]

The AppVeyor exceeded 60 min time allowed but as Nima said, perhaps broken.

[nhejazi]

The Appveyor build is failing because currently appveyor.yml is set to treat warnings as errors, which causes the build to fail. The warning that is treated as an error is from one of the tests, and it goes back to cv.glmnet. Here are the relevant warning messages:

Warning message:
running command '"c:/R/bin/x64/R" CMD BATCH --vanilla  "testthat.R" "testthat.Rout"' had status 1 
 ERROR
Running the tests in 'tests/testthat.R' failed.
Last 13 lines of output:
  Calls: test_check ... eval -> eval -> hal -> <Anonymous> -> glmnet -> match.arg
  In addition: Warning messages:
  1: Option grouped=FALSE enforced in cv.glmnet, since < 3 observations per fold 
  2: Option grouped=FALSE enforced in cv.glmnet, since < 3 observations per fold 
  3: Option grouped=FALSE enforced in cv.glmnet, since < 3 observations per fold 
  4: Option grouped=FALSE enforced in cv.glmnet, since < 3 observations per fold 
  5: Option grouped=FALSE enforced in cv.glmnet, since < 3 observations per fold 
  6: Option grouped=FALSE enforced in cv.glmnet, since < 3 observations per fold 
  7: Option grouped=FALSE enforced in cv.glmnet, since < 3 observations per fold 
  8: Option grouped=FALSE enforced in cv.glmnet, since < 3 observations per fold 
  9: Option grouped=FALSE enforced in cv.glmnet, since < 3 observations per fold 
  10: Option grouped=FALSE enforced in cv.glmnet, since < 3 observations per fold 
  testthat results ================================================================
  OK: 0 SKIPPED: 0 FAILED: 0
  Execution halted
* DONE

You can fix this by changing the line (nested under environment, then global):
WARNINGS_ARE_ERRORS: 1 to WARNINGS_ARE_ERRORS: 0, which will stop warnings from being treated as errors. Alternatively, I guess you could just increase the sample size used for the test that is generating that warning.

For now, you should probably feel free to merge your PR since it is passing Travis and it does not appear that the commits contained in your PR are the reason the Appveyor builds are failing.

[jlstiles]

Thanks, Nima. Well then it should be OK up to superficialities. I'll leave the test parameters up to the contributors.