search

leeper / prediction

Tidy, Type-Safe 'prediction()' Methods
https://cran.r-project.org/package=prediction
Other
89 stars 14 forks source link

Support for crch package #4

Closed carlganz closed 7 years ago

carlganz commented 7 years ago

Just a small addition.

Unrelated, but I noticed that you used model.frame inside of find_data.merMod. It occurs to me now that model.frame is a generic function that essentially serves the same purpose as the generic find_data. I could be wrong, but I am pretty sure we could substitute model.frame for find_data without any problems.

codecov-io commented 7 years ago

Current coverage is 8.97% (diff: 0.00%)

Merging #4 into master will decrease coverage by 0.68%

@@            master        #4   diff @@
========================================
  Files            6         6          
  Lines          290       312    +22   
  Methods          0         0          
  Messages         0         0          
  Branches         0         0          
========================================
  Hits            28        28          
- Misses         262       284    +22   
  Partials         0         0

Powered by Codecov. Last update b7cd9f9...f466ea4

leeper commented 7 years ago

Thanks, I'll take a look at this soon.

To answer your question, though, model.frame() is not a good generic solution. The problem is that if a model specification modifies its input variables, model.frame() will return the modified versions rather than the original versions. It is the latter (the original form) that is needed for predict() methods. See, for example:

str(model.frame(m1 <- lm(mpg ~ factor(cyl), data = mtcars)))
str(model.frame(m2 <- lm(mpg ~ I(wt*100), data = mtcars)))
predict(m1, model.frame(m1))
predict(m2, model.frame(m2))
carlganz commented 7 years ago

I'll update this request to comply with the code of conduct. Apologies for not filing an issue first.

Thanks for the explanation. Makes sense.

leeper commented 7 years ago

No worries at all. It looks like a good addition on quick glance.

leeper commented 7 years ago

Thanks. This looks good. I'm going to rebase and merge. You'll need to update your fork accordingly.

carlganz commented 7 years ago

Hey, turns out model.frame won't work for crch for the reasons you mentioned above. Will fix ASAP. For some reason when I used default find_data with crch on Friday I found that if the dataset was named data it would get angry, but I can't reproduce the problem now, so I am pretty confident default find_data is fine for crch.