zachmayer / caretEnsemble

caret models all the way down :turtle:
http://zachmayer.github.io/caretEnsemble/
Other
226 stars 75 forks source link

Release fixes #73

Closed jknowles closed 9 years ago

jknowles commented 9 years ago

This PR closes #71 #44 and #72. It also revamps the test suite to reflect the new desired behavior of predict.caretEnsemble.

zachmayer commented 9 years ago

I see an R CMD CHECK warnings and note in travis:

Codoc mismatches from documentation object 'predict.caretEnsemble':
predict.caretEnsemble
  Code: function(object, keepNA = TRUE, se = NULL, return_weights =
                 FALSE, ...)
  Docs: function(object, keepNA = TRUE, se = NULL, ...)
  Argument names in code not in docs:
    return_weights
  Mismatches in argument names:
    Position: 4 Code: return_weights Docs: ...

and

Examples with CPU or elapsed time > 5s
            user system elapsed
caretStack 7.703  0.061   8.373

I think re-running document() will fix the warning. I can probably take charge of fixing that note. I didn't get it on my system =/

zachmayer commented 9 years ago

So here's the code I ran to test this (do we have test cases for each of these?):

predict(ens.reg, keepNA = FALSE, se = FALSE, return_weights = FALSE)
predict(ens.reg, keepNA = TRUE, se = FALSE, return_weights = FALSE)
predict(ens.reg, keepNA = FALSE, se = TRUE, return_weights = FALSE)
predict(ens.reg, keepNA = TRUE, se = TRUE, return_weights = FALSE)
predict(ens.reg, keepNA = FALSE, se = FALSE, return_weights = TRUE)
predict(ens.reg, keepNA = TRUE, se = FALSE, return_weights = TRUE)
predict(ens.reg, keepNA = FALSE, se = TRUE, return_weights = TRUE)
predict(ens.reg, keepNA = TRUE, se = TRUE, return_weights = TRUE)

The behavior is: If se=FALSE and return_weights = FALSE, always return a vector. If se=TRUE and return_weights = FALSE, always return a data.frame. If return_weights = TRUE, always return a list. The first element of the list is a vector or data.frame depending on se.

zachmayer commented 9 years ago

Actually, I ended up writing unit tests that check all the cases. Can you add this code to your PR?

#Reg tests
load(system.file("testdata/models_reg.rda", package="caretEnsemble", mustWork=TRUE))
ens.reg <- caretEnsemble(models_reg, iter=1000)
tests <- expand.grid(keepNA=0:1, se=0:1, return_weights=0:1)
tests <- data.frame(lapply(tests, as.logical))
for(i in 1:nrow(tests)){
  p <- predict(
    ens.reg, 
    keepNA=tests[i,'keepNa'],
    se=tests[i,'se'],
    return_weights=tests[i,'return_weights']
  )

  if(tests[i,'return_weights']){
    expect_is(p, 'list')
    preds <- p$preds
  } else{
    preds <- p
  }

  if(tests[i,'se']){
    expect_is(preds, 'data.frame')
  } else{
    expect_is(preds, 'numeric')
  }

}

#Class tests
load(system.file("testdata/models_class.rda", package="caretEnsemble", mustWork=TRUE))
ens.class <- caretEnsemble(models_class, iter=1000)
tests <- expand.grid(keepNA=0:1, se=0:1, return_weights=0:1)
tests <- data.frame(lapply(tests, as.logical))
for(i in 1:nrow(tests)){
  p <- predict(
    ens.class, 
    keepNA=tests[i,'keepNa'],
    se=tests[i,'se'],
    return_weights=tests[i,'return_weights']
  )

  if(tests[i,'return_weights']){
    expect_is(p, 'list')
    preds <- p$preds
  } else{
    preds <- p
  }

  if(tests[i,'se']){
    expect_is(preds, 'data.frame')
  } else{
    expect_is(preds, 'numeric')
  }

}

Or... How do I add commits to your PR?

zachmayer commented 9 years ago

This all looks great. I have a few comments.

jknowles commented 9 years ago

I did your suggestions. We'll let Travis do it's magic now!

jknowles commented 9 years ago

Also that is a really elegant unit test structure you wrote. I will be able to prune down the test suite significantly following that pattern. I'll look to do that in a future PR.

zachmayer commented 9 years ago

Awesome. Yeah, a unit test cleanup is a task for a future day.