aldro61 / mmit

Regression trees for interval censored output data
https://aldro61.github.io/mmit/
GNU General Public License v3.0
7 stars 7 forks source link

Cv default predict #39

Closed tdhock closed 4 years ago

tdhock commented 5 years ago

Hi @parismita I have a suggestion for the user interface of the *.cv functions.

Typically a useR would expect to be able to do

fit <- compute_model(X, y)
pred.vec <- predict(fit, Xtest)

in order to get a vector of predictions for a test data set.

This can not be done using the current mmit.cv/mmif.cv functions (I coded two tests that should fail).

There are two issues that should be fixed:

  1. *.cv(X,y) gives an error: param_grid is missing with no default. Can you please provide some sensible defaults?
  2. predict(fit, Xtest) does not work on objects returned from *.cv functions. Can you please assign classes "mmit.cv" and "mmif.cv" to the objects returned by those functions, and define appropriate S3 methods? Full docs are here http://adv-r.had.co.nz/S3.html but essentially you just need to define two new functions, named predict.mmit.cv and predict.mmif.cv, which compute the vector of predictions, for example https://github.com/tdhock/penaltyLearning/blob/master/R/IntervalRegression.R#L710
parismita commented 5 years ago

yeah...I'll work on the second issue

regarding the param_grid, the default values are already assigned inside mmit.cv/mmif.cv, so we can assign param_grid = NULL in the argument to solve the 1st issue.

tdhock commented 5 years ago

ok great in that case the fixes should be easy.

parismita commented 5 years ago

yeah

parismita commented 4 years ago

@tdhock Hi, the two tasks are completed, can you review it, I need to pull the latest changes into the Adaboost PR to complete that one. Thanks

tdhock commented 4 years ago

hi @parismita thanks for the update. On Travis-R I see the following issues related to docs / method declarations, can you please fix?

* checking S3 generic/method consistency ... WARNING
predict:
  function(object, ...)
predict.mmif:
  function(forest, test_feature.mat)
predict:
  function(object, ...)
predict.mmif.cv:
  function(object, newdata)
predict:
  function(object, ...)
predict.mmit:
  function(tree, newdata, perm)
predict:
  function(object, ...)
predict.mmit.cv:
  function(object, newdata, perm)
See section ‘Generic functions and methods’ in the ‘Writing R
Extensions’ manual.

for these above you need to make sure that your predict functions (e.g. predict.mmit) always have the dots arguments at the end (even if they are ignored) in order to be consistent with the generic which has those dots. also for function(forest, test_feature.mat) you should change "forest" to "object" to be consistent with the generic. for example this is what I do for a custom predict method https://github.com/tdhock/penaltyLearning/blob/master/R/IntervalRegression.R#L730 which does not generate any warnings.

* checking for code/documentation mismatches ... WARNING
Codoc mismatches from documentation object 'predict.mmif.cv':
predict.mmif.cv
  Code: function(object, newdata = NULL)
  Docs: function(object, newdata = NULL, perm = NULL)
  Argument names in docs not in code:
    perm

for these looks like you probably just need to run devtools::document()

parismita commented 4 years ago

ok...I'll check that, maybe I forgot to update the doc for the predict functions

parismita commented 4 years ago

@tdhock I updated the docs and solved that issue, it was because I didn't define S3method in namespace.

tdhock commented 4 years ago

ok looks good to me @parismita since I initiated the PR, somebody else needs to review before it can be merged (I think), can you please review + merge?

parismita commented 4 years ago

ok