-
Notifications
You must be signed in to change notification settings - Fork 75
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
Adding support for custom models #198
Conversation
tests/testthat/test-ensemble.R:212:3: style: Words within variable and function names should be separated by '_' rather than '.'. X.class.df <- as.data.frame(X.class)
^~~~~~~~~~ tests/testthat/test-ensemble.R:215:34: style: Words within variable and function names should be separated by '_' rather than '.'. expect_warning(cl <- caretList(X.class.df, Y.class, tuneList=tune.list, trControl=train.control))
^~~~~~~~~~ tests/testthat/test-ensemble.R:225:54: style: Words within variable and function names should be separated by '_' rather than '.'. expect_silent(pred.classb <- predict(cs, newdata = X.class.df, type="prob"))
^~~~~~~~~~ tests/testthat/test-ensemble.R:226:54: style: Words within variable and function names should be separated by '_' rather than '.'. expect_silent(pred.classc <- predict(cs, newdata = X.class.df[2,], type="prob"))
^~~~~~~~~~ tests/testthat/test-ensemble.R:226:67: style: Commas should always have a space after. expect_silent(pred.classc <- predict(cs, newdata = X.class.df[2,], type="prob"))
^ |
Looks good to me. Please fix the lint errors, and I'll do a final review and merge. |
bd2872d
to
cae8cdc
Compare
Will do (I'm traveling so sorry for the delay). I don't know why those lint errors aren't all showing up when running the local tests. Oh well I'll get them fixed eventually. |
tests/testthat/test-ensemble.R:226:61: style: Commas should always have a space after. expect_silent(pred.classc <- predict(cs, newdata = X.df[2,], type="prob"))
^ |
395340f
to
d1d5742
Compare
@zachmayer do you have any idea why this build keeps failing? I can't really tell just based on the TravisCI logs. I fixed all the lint errors (and I keep squashing those changes down to keep everything in one commit). That most recent issue with the build mentioned by lintr-bot was definitely fixed so I'm at a loss. |
Taking a look at the travis logs, I see one warning and one note: * checking R code for possible problems ... NOTE
methodCheck: no visible binding for global variable ‘type’ ... * checking Rd \usage sections ... WARNING
Undocumented arguments in documentation object 'validateCustomModel'
‘x’
Documented arguments not in \usage in documentation object 'validateCustomModel':
‘a’ The warning is causing the test failure— I think you just need to update the You can run these checks locally with |
validateCustomModel(x) | ||
} | ||
\arguments{ | ||
\item{a}{model info list (e.g. getModelInfo("rf", regex=F)[[1]])} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This miss-match is what's causing the text failure. The argument of the function is x
and the argument you documented is a
.
HOWEVER, in the roxygen comment block, the documentation looks correct to me. I think running devtools::document()
and pushing the result will fix the problem.
I added some comments that I think will help you fix the problem. Please let me know if you have any other issues, and I'll help you work them out! |
d1d5742
to
164c74e
Compare
Changes Unknown when pulling 164c74e on eric-czech:adding_custom_models into * on zachmayer:master*. |
Beautiful, thanks for the assist! |
No problem, it's what I'm here for lol! |
Adding support for custom models
Hi @zachmayer , here is a PR for custom model support with some more docs and at least one decent test. Let me know if you have any suggestions.
Also for the sake of reference, here is some example usage:
ps I closed the first PR for this after a bad merge on that branch, and had to create this one instead.