Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix prediction_glm.R newdata argument #33

Merged
merged 1 commit into from Apr 6, 2019

Conversation

Projects
None yet
2 participants
@vincentarelbundock
Copy link
Contributor

vincentarelbundock commented Mar 10, 2019

This is a minor bugfix (with a new test) for: #32

I'm not sure how your version numbers work, so I just added a new entry to NEWS.

library(prediction)
data(mtcars)

# `data` with lm works
mod <- lm(am ~ hp + drat, data = mtcars)
prediction(mod, data = data.frame(hp = 110, drat = 3.9))
#> Data frame with 1 predictions from
#>  lm(formula = am ~ hp + drat, data = mtcars)
#> with average prediction: 0.5947

# `data` with glm doesn't work
mod <- glm(am ~ hp + drat, data = mtcars)
prediction(mod, data = data.frame(hp = 110, drat = 3.9))
#> Warning in model$family$mu.eta(predictions_link) * model_mat: longer object
#> length is not a multiple of shorter object length
#> Error in is.data.frame(x): dims [product 3] do not match the length of object [32]

This problem was caused by calling predict(data = ) instead of predict(newdata = in a couple places in prediction_glm.R.

Please ensure the following before submitting a PR:

  • if suggesting code changes or improvements, open an issue first
  • for all but trivial changes (e.g., typo fixes), add your name to DESCRIPTION
  • for all but trivial changes (e.g., typo fixes), documentation your change in NEWS.md with a parenthetical reference to the issue number being addressed
  • if changing documentation, edit files in /R not /man and run devtools::document() to update documentation
  • add code or new test files to /tests for any new functionality or bug fix
  • make sure R CMD check runs without error before submitting the PR
@vincentarelbundock

This comment has been minimized.

Copy link
Contributor Author

vincentarelbundock commented Apr 6, 2019

@leeper When you have a second, do you think you could take a look at this one? This PR would allow me to fix Travis failures and finish up work downstream in margins for my ggplot WIP PR.

I only modified 2 lines of actual code and added a test, so it shouldn't be very difficult to review. I also added my name as contributor, but this is so minor that I would be happy to remove it.

Travis works in 5 of the 6 cases; the only case where it fails relates to package installation in an old R release, so it has nothing to do with this commit.

@leeper

This comment has been minimized.

Copy link
Owner

leeper commented Apr 6, 2019

Looks great. Thanks!

@leeper leeper merged commit 96071dd into leeper:master Apr 6, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@vincentarelbundock

This comment has been minimized.

Copy link
Contributor Author

vincentarelbundock commented Apr 6, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.