Skip to content
Please note that GitHub no longer supports your web browser.

We recommend upgrading to the latest Google Chrome or Firefox.

Learn more
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

Fix "vce" bug with length 1 "variables" argument #113

Merged
merged 5 commits into from Dec 23, 2019
Merged

Conversation

@jacob-long
Copy link
Contributor

jacob-long commented Feb 17, 2019

Please ensure the following before submitting a PR:

  • if suggesting code changes or improvements, open an issue first

Closes #112

  • for all but trivial changes (e.g., typo fixes), add your name to DESCRIPTION

You can decide whether this exceeds the triviality threshold.

  • 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

There are tests failing (well, throwing warnings) on my machine, but they don't appear to be related to this issue.

@jacob-long

This comment has been minimized.

Copy link
Contributor Author

jacob-long commented Feb 17, 2019

I meant to add:

Another strategy is to not make any use of variables until the last possible moment and just subset the summary/print output. This would have a performance cost, though:

library(margins)
fit <- lm(mpg ~ wt + hp * cyl, data = mtcars)
microbenchmark::microbenchmark(
  margins(fit, vce = "bootstrap"),
  margins(fit, vce = "bootstrap", variables = "hp")
)
#> Unit: milliseconds
#>                                               expr      min       lq      mean     median       uq      max neval cld
#>                    margins(fit, vce = "bootstrap") 175.1137 180.3685 184.77140  184.57105 187.6936 218.1735   100   b
#>  margins(fit, vce = "bootstrap", variables = "hp")  89.4291  93.0809  98.03813   96.67365 101.9262 116.3726   100  a    

microbenchmark::microbenchmark(
  margins(fit, vce = "simulation"), 
  margins(fit, vce = "simulation", variables = "hp")
)
#> Unit: milliseconds
#>                                                expr      min       lq      mean   median       uq      max neval cld
#>                    margins(fit, vce = "simulation") 131.5059 135.8638 143.24005 141.6665 145.0785 315.2456   100   b
#>  margins(fit, vce = "simulation", variables = "hp")  49.8094  51.6491  54.30603  52.4074  55.9454  79.2443   100  a 

So I think going in and fixing it in the general way I've done is best, especially since the total amount of new code is probably not that much different.

@leeper

This comment has been minimized.

Copy link
Owner

leeper commented Feb 17, 2019

Looks like something broke in the Introduction vignette?

@leeper leeper added the bug label Feb 17, 2019
@jacob-long

This comment has been minimized.

Copy link
Contributor Author

jacob-long commented Feb 17, 2019

Quitting from lines 51-52 (Introduction.Rmd) 
Error in margins_summary(x) : could not find function "margins_summary"
Calls: <Anonymous> ... handle -> withCallingHandlers -> withVisible -> eval -> eval

Edit: Oops, that's only with the CRAN version installed.

@jacob-long

This comment has been minimized.

Copy link
Contributor Author

jacob-long commented Feb 17, 2019

I might suggest clearing your cache on Travis, this has resolved issues for me in the past. The issue on Appveyor is unrelated.

@leeper leeper merged commit f4054c5 into leeper:master Dec 23, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@leeper

This comment has been minimized.

Copy link
Owner

leeper commented Dec 23, 2019

Thanks!

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