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

stylistic changes #439

Merged
merged 7 commits into from
Mar 10, 2021
Merged

stylistic changes #439

merged 7 commits into from
Mar 10, 2021

Conversation

IndrajeetPatil
Copy link
Member

No description provided.

@strengejacke
Copy link
Member

sadly, this option doesn't seem to work on Windows...

library(parameters)
model <- lm(mpg ~ wt + cyl, data = mtcars)

microbenchmark::microbenchmark(
  model_parameters(model, bootstrap = TRUE, iterations = 1000, parallel = "snow"),
  times = 5
)
#> Unit: seconds
#>                                                                                  expr
#>  model_parameters(model, bootstrap = TRUE, iterations = 1000,      parallel = "snow")
#>       min       lq     mean   median       uq      max neval
#>  1.072621 1.076588 1.141162 1.108037 1.109053 1.339513     5

microbenchmark::microbenchmark(
  model_parameters(model, bootstrap = TRUE, iterations = 1000, parallel = "no"),
  times = 5
)
#> Unit: seconds
#>                                                                                expr
#>  model_parameters(model, bootstrap = TRUE, iterations = 1000,      parallel = "no")
#>       min       lq     mean   median       uq      max neval
#>  1.050956 1.060096 1.118408 1.064866 1.070488 1.345632     5

microbenchmark::microbenchmark(
  model_parameters(model, bootstrap = TRUE, iterations = 1000, parallel = "multicore"),
  times = 5
)
#> Unit: seconds
#>                                                                                       expr
#>  model_parameters(model, bootstrap = TRUE, iterations = 1000,      parallel = "multicore")
#>       min       lq     mean   median       uq      max neval
#>  1.083162 1.138866 1.161232 1.176707 1.188325 1.219101     5

Created on 2021-03-09 by the reprex package (v1.0.0)

@IndrajeetPatil
Copy link
Member Author

I will merge for now, and we can build on it further later.

@strengejacke
Copy link
Member

But checks don't pass.

@IndrajeetPatil
Copy link
Member Author

Uh-oh, let me check what happened.

@strengejacke
Copy link
Member

Looks like an invalid argument was used in the tests, which didn't do any harm, because we didn't pass down the dots to boot() ;-)
When we use boostrapping, dots may only contain valid arguments for boot().

@strengejacke
Copy link
Member

@DominiqueMakowski This PR breaks a lot of other stuff, in particular all parameters that are passed down to bayestestR::describe_posterior(). Not sure what's the best way to go... Maybe we can capture the dots, then "filter" for valid arguments for boot(), and only pass those valid arguments to boot() (via do.call()).

@IndrajeetPatil would you revise this PR accordingly to not break current functionality?

@DominiqueMakowski
Copy link
Member

Maybe we can capture the dots, then "filter" for valid arguments

wouldn't something like parallel <- ifelse("parallel" %in% names(as.list(...)), as.list(...)$parallel, "no") and then pass parallel to boot work?

@IndrajeetPatil
Copy link
Member Author

I prefer to then just go back to the earlier implementation I had in mind:
explicitly providing parallel and ncpus argument, which, yes, complicates the API slightly, but would not cause any problems for any of the methods.

@IndrajeetPatil IndrajeetPatil changed the title Issue 436 stylistic changes Mar 10, 2021
@IndrajeetPatil
Copy link
Member Author

As discussed in #436, I've decided to revert all boot-related changes. Now the PR only contains stylistic 80-width changes.

@IndrajeetPatil IndrajeetPatil merged commit 6ca0344 into easystats:main Mar 10, 2021
@IndrajeetPatil IndrajeetPatil deleted the issue_436 branch March 10, 2021 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants