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

Add threading for variational/optimize #369

Merged
merged 8 commits into from
Nov 30, 2020
Merged

Add threading for variational/optimize #369

merged 8 commits into from
Nov 30, 2020

Conversation

rok-cesnovar
Copy link
Member

Summary

Fixes #193

Copyright and Licensing

Please list the copyright holder for the work you are submitting
(this will be you or your assignee, such as a university or company):
Rok Češnovar, Uni. of Ljubljana

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses:

R/model.R Outdated
@@ -1024,7 +1029,23 @@ optimize_method <- function(data = NULL,
algorithm = NULL,
init_alpha = NULL,
iter = NULL,
sig_figs = NULL) {
sig_figs = NULL,
threads = NULL) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not so sure about the name here. but threads_per_chain does not work for optimize/variational.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's a good question. Ideally we would use the same argument name but you're right that there are no chains here. I guess threads is fine. @mitzimorris or @bbbales2 any thoughts on what name we should use here?

R/model.R Show resolved Hide resolved
R/model.R Show resolved Hide resolved
@jgabry
Copy link
Member

jgabry commented Nov 25, 2020

I put a few comments, but this looks good to me, thanks!

@rok-cesnovar
Copy link
Member Author

Thanks. I guess the only thing to discuss is whether threads = X is a good argument for optimization/variational. It's a different name than sampling, but threads_per_chain does not work for optimization.

@jgabry
Copy link
Member

jgabry commented Nov 26, 2020

Thanks. I guess the only thing to discuss is whether threads = X is a good argument for optimization/variational. It's a different name than sampling, but threads_per_chain does not work for optimization.

I think threads is good but we can wait a little while to see if anyone has a different suggestion.

@codecov-io
Copy link

codecov-io commented Nov 26, 2020

Codecov Report

Merging #369 (75e7e37) into master (b9ce4d9) will decrease coverage by 2.21%.
The diff coverage is 72.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #369      +/-   ##
==========================================
- Coverage   90.58%   88.37%   -2.22%     
==========================================
  Files          12       12              
  Lines        2571     2614      +43     
==========================================
- Hits         2329     2310      -19     
- Misses        242      304      +62     
Impacted Files Coverage Δ
R/model.R 88.79% <69.04%> (-3.56%) ⬇️
R/read_csv.R 97.56% <100.00%> (-1.21%) ⬇️
R/run.R 96.55% <100.00%> (-2.30%) ⬇️
R/install.R 43.54% <0.00%> (-8.41%) ⬇️
R/utils.R 92.20% <0.00%> (-1.30%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9ce4d9...75e7e37. Read the comment docs.

@bbbales2
Copy link
Member

I'm happy with threads. I hope in the future we do a multiple-runs sorta VI (everything should be checked -- https://arxiv.org/abs/2009.00666), but that isn't here now.

@jgabry
Copy link
Member

jgabry commented Nov 27, 2020

Yeah I think multiple-run VI would be a good thing in the future. I guess we can adjust as necessary when that time comes.

@jgabry jgabry merged commit f32e13c into master Nov 30, 2020
@jgabry jgabry deleted the threading branch November 30, 2020 23:01
@jgabry
Copy link
Member

jgabry commented Nov 30, 2020

Seems like we're going with threads at least until there's multi-run VI. That seems fine so I'm going to go ahead merge this because the only thing holding it up was that argument name.

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.

Threading interface for optimization/VI
4 participants