leeper / prediction

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

Allow binding of predictions with original data #8

Closed benwhalley closed 7 years ago

benwhalley commented 7 years ago

I wonder if you'd welcome a little PR which added a bind argument to prediction which then included the predictions with the original dataset? Something like: https://github.com/benwhalley/prediction/commit/13020c9c1ab04354d575d9dadf968542b4615065

It only saves a line or two of code, but for students I think will be helpful when plotting models:

library(tidyverse)
m <- lm(mpg ~ drat*factor(cyl), data = mtcars)

newd <- expand.grid(
  hp=median(mtcars$hp), 
  cyl=unique(mtcars$cyl),
  drat=seq_range(range(mtcars$drat), 10)
)

preds <- prediction(m, newd, bind=T)

ggplot(preds, aes(drat, fitted, group=cyl, color=factor(cyl))) + 
      geom_smooth(se=F)

My preference would be for the default to be bind=T, but realise this might break things.

leeper commented 7 years ago

It probably makes sense for this to be the default. It has a couple of consequences downstream in the margins package that will need to be adjusted (otherwise the columns will all be duplicated multiple times).

benwhalley commented 7 years ago

Thanks for this... out of interest, is there a reason why you didn't want to implement in a single place by wrapping the function? I'm still getting to grips with what is idiomatic R.

Related to this issue, I had wondered if you could do something similar with the summary method of margins(). For example, say you had a situation like this:

m <- glm(satstill~sticker*lollipop*agemonths, data=dentist, family="binomial")

m.margins <- margins(m, at=list(agemonths=c(50,80,90), lollipop=0:1))

It would be nice if summary had the following automatically applied to it, so that it returned a tidy dataframe rather than a list:

# new return value for summary.margins
do.call('rbind', lapply(summary(m.margins), 
          function(x) {cbind(attr(x, 'at'), x)})) %>% 
  as_tibble() 
leeper commented 7 years ago

Can you move that suggestion over to the margins repo? Seems that something like that could work.

The logic here is to not mix S3 class dispatch and behavior. prediction() should just dispatch to methods, not actually do anything. See, for example, the definition of print().