SteffenMoritz / ridge

CRAN R Package: Ridge Regression with automatic selection of the penalty parameter
GNU General Public License v2.0
18 stars 11 forks source link

predict.linearRidge does not work if formula goes out of scope #7

Closed dfrankow closed 4 years ago

dfrankow commented 4 years ago

Test case:

library(testthat)

tol <- 0.0001

test_that("test linearRidge with formula variable that goes out of scope", {
  the_formula <- formula('mpg ~ wt + cyl')
  model1 <- lm(the_formula, data = mtcars)
  model2 <- linearRidge(the_formula, data = mtcars, lambda = 0)
  # suppose the_formula goes away:
  rm(the_formula)

  expect_equal(coef(model1), coef(model2), tolerance = tol, label = "coefficients")
  expect_equal(predict(model1), predict(model2), tolerance = tol, label = "predictions")
})

This is separate from my changes, and also fails on 2.4.

dfrankow commented 4 years ago

The failure message:

Error: Test failed: 'test linearRidge with formula variable that goes out of scope'
* object 'the_formula' not found
Backtrace:
 1. stats::model.frame(formula = the_formula, data = mtcars)
dfrankow commented 4 years ago

lm handles this, I think by saving the environment somehow. It has tons of specialized functions, including model.matrix.lm and model.frame.lm that set up and move around data.

dfrankow commented 4 years ago

This issue is important because if a linearRidge model is set up in a function (that returns the model, say), when that function returns, everything in it will go out of scope, and the model won't be able to predict.

SteffenMoritz commented 4 years ago

Wow, does this pull request already fix the issue? You are so fast 🥇 I have to go to bed now ... hope I can run my tests tomorrow. Today I was more busy with fixing my Mac issues...and was not able to actually test much.

dfrankow commented 4 years ago

Glad you think so. Thanks for your quick response.

These issues are blocking me, so I've been working on it a bunch. However, work at your own pace. In a pinch I can use my fork, so your release is not blocking me.