Update on LFDA using new lfda package on CRAN

[terrytangyuan]

No description provided.

[zachmayer]

It looks like you have an error in your tests.

[terrytangyuan]

Yeah I wasn't able to figure out why. Let me know if you found anything I missed.

[topepo]

Since you now have these functions in your own R package, I'm not sure why you are still committing to this package.

[zachmayer]

@topepo He needs to revise the lfda model in caret to use his new package

[zachmayer]

Remove this import. We don't want caret to depend on modeling packages.

[zachmayer]

@terrytangyuan Please delete the lfda.R and lfda.default.Rd files too.

[zachmayer]

@topepo This PR should remove all lfda-related code, as it's migrated to @terrytangyuan's new package on CRAN.

[zachmayer]

@terrytangyuan You can also add a lfda.R file to the models section so it can be run in base caret. Inside that file you can specify that the model depends on the new lfda package.

[terrytangyuan]

@zachmayer Just double check: so I should remove all dependencies and code related to lfda and then write a new file to the models section, correct?

[zachmayer]

Correct. That's the preferred way to add new models to caret: no code or dependencies live inside the package, just a custom model definition that tells caret which model to load.

[terrytangyuan]

@zachmayer I tried adding S3method(predict, lfda) to NAMESPACE but predict on lfda didn't work (if you uncomment the lines related to predict in testthat\test_models_lfda.R). I'll need to add predict method to lfda package later but any workaround with this so predict can be used in caret now? I believe I missed something important.

[zachmayer]

On this PR, edit the .travis.yml file. Find this line:
{yaml}

• ./travis-tool.sh install_github jimhester/covr

And add this below it
```{yaml}```
  - ./travis-tool.sh install_github terrytangyuan/lfda

This will install lfda from github. Then you'll need to add predict.lfda to your github package. Make sure to tag it with:

@export
@method predict lfda

Here's what the full roxygen2 block for lfda might look like:

@title predict.lfda
@description make predictions for new data from an lfda object
@param object An lfda object
@param newdata Data to use for predictions
@param ... ignored

[zachmayer]

This test should move to your own package, as it JUST tests the lfda model.

[zachmayer]

Finally, tests for lfda and predict.lfda belong in your own package. Change your test here to be something like:

test_that('test lfda model training and prediction', {
  skip_on_cran()
  set.seed(1)
  tr_dat <- twoClassSim(200)
  te_dat <- twoClassSim(200)

  model <- train(
    Class~., tr_dat, 
    method = "lfda", 
    trControl = trainControl(method='cv', number='5')
    )
  expect_is(model, 'train')
  expect_is(model$finalModel, 'lfda')
  expect_is(model$results$Accuracy, 'numeric')
  expect_true(model$results$Accuracy >= .75)
})

[zachmayer]

This is always going to cause your model to fail. Caret models ALWAYS need a grid. See here for a simple modelInfo you can copy: https://github.com/terrytangyuan/caret/blob/newlfda/models/files/lda.R

[zachmayer]

Also, take a quick read through here: http://topepo.github.io/caret/custom_models.html

[terrytangyuan]

Thank you! These are very helpful. I'll read through and revise this. I've missed a lot of stuff.

[terrytangyuan]

@zachmayer I kept getting this Model lfda is not in caret's built-in library. What should I change to include it?

[zachmayer]

You need to re-run the script to create the models data object: https://github.com/topepo/caret/blob/master/models/parseModels.R

[zachmayer]

Ok, we'll see if this build passes on master and I'll make a PR to fix it.

[zachmayer]

For future reference, you need to keep your fork up-to-date with topepo/caret master. This will reduce the likelihood of merge conflicts on future PRs.

Basically, what happened was Max added a new model at the same time you did. This created a conflict.

The resolution of the conflict is to merge mx's branch into yours and re-create models.RData such that it includes both models (max's new model and your changed one)

[zachmayer]

For future reference, you can test your model as follows:

modelInfo <- list(label = "Local Fisher Discriminant Analysis",
                  library = c("lfda"),
                  type = "Classification",
                  grid = function(x, y, len = NULL, search = "grid") data.frame(r="none",metric="none", knn="none"),
                  parameters = data.frame(parameter = c("r", "metric", "knn"),
                                          class = c("numeric", "character", "numeric"),
                                          label = c("# Reduced Dimensions",
                                                    "Type of Transformation Metric",
                                                    "# of Nearest Neighbors")),
                  fit = function(x, y, param, ...) {
                    theDots <- list(...)

                    argList <- list(x = x, y = y, r = ifelse(is.null(param$r, 3, param$r)),
                                    metric = ifelse(is.null(param$metric), "plain", param$metric),
                                    knn = ifelse(is.null(param$knn, 5, param$knn)))
                    argList <- c(argList, theDots)

                    if(is.data.frame(x)) x <- as.matrix(x)

                    out <- do.call("lfda", argList)

                    out$call <- NULL
                    out
                  },
#                 predict = function(modelFit, newdata, submodels = NULL)
#                   predict(modelFit, newdata),
                  prob = NULL,
                  predictors = function(x, ...) {
                    # if dimensionality of original data is not reduced
                    if(dim(x$T)[1]==dim(x$T)[2]){
                      return(colnames(x$Z))
                    } else {
                      print("predictors are not available for lfda model with dimension reduction. ")
                      return(NULL)
                    }
                  },
                  tags = c("Metric Learning", "Local Metric Learning", "Dimension Reduction",
                           "Multimodality Preservance", "Fisher Discriminant Analysis",
                           "Classification", "Pre-processing")
                  )

  library(lfda)
  tr_dat <- twoClassSim(200)
  te_dat <- twoClassSim(200)

  lfda.model <- train(
    x=tr_dat[,-16],y=tr_dat[,16],
    method = modelInfo
  )

As you can see this fails. Digging deeper, we can see the fit function is failing:

modelInfo$fit(tr_dat[,-16], tr_dat[,16], modelInfo$grid(1))

I'm making a PR to fix this, but in the future you need to test your models and make sure they work before submitting them. You get a freebie this time since there were git issues to work out =D

Next time you should be able to do this on your own.

[zachmayer]

I can't actually get lfda to run on any input data: lfda(matrix(runif(100), ncol=10), runif(10), r=3):

5 stop("'x' must be an array of at least two dimensions") 
4 colSums(Xc^2) 
3 as.matrix(colSums(Xc^2)) 
2 t(as.matrix(colSums(Xc^2))) 
1 lfda(matrix(runif(100), ncol = 10), runif(10), r = 3) 

Can you send me an example of how to run lfda, e.g. on the iris dataset?

[terrytangyuan]

I am not by my pc but you can check the tests in lfda package.
On Aug 14, 2015 4:00 PM, "Zach Mayer" notifications@github.com wrote:

I can't actually get lfda to run on any input data: lfda(matrix(runif(100),
ncol=10), runif(10), r=3):

5 stop("'x' must be an array of at least two dimensions") 4 colSums(Xc^2) 3 as.matrix(colSums(Xc^2)) 2 t(as.matrix(colSums(Xc^2))) 1 lfda(matrix(runif(100), ncol = 10), runif(10), r = 3)

Can you send me an example of how to run lfda, e.g. on the iris dataset?

—
Reply to this email directly or view it on GitHub
#219 (comment).

[zachmayer]

What data type do the arguments to lfda need to be?

[zachmayer]

Ok, go it figured out. PR coming tongiht.

[terrytangyuan]

Thanks! And yep I got the lesson.