Added parallel likelihood computation

[kdubovikov]

Hi. I have added parallel likelihood computation to speedup loo_subsample when using posterior approximations based on this discussion https://discourse.mc-stan.org/t/problem-running-loo-subsample-with-variational-inference-and-cmdstanr/22506/18.

CC @MansMeg

[jgabry]

Thanks for the PR! Will let @MansMeg review since he is more familiar with loo_subsample(). @MansMeg let me know if you also want me to look at any particular parts of this (or all of it).

[jgabry]

@kdubovikov If you run the tests for loo subsample locally do they pass for you? It looks like we're getting a bunch of test failures when checking online but that may or may not be related to this PR (I haven't had a chance to investigate).

[kdubovikov]

@kdubovikov If you run the tests for loo subsample locally do they pass for you? It looks like we're getting a bunch of test failures when checking online but that may or may not be related to this PR (I haven't had a chance to investigate).

Yes, tests are failing with NULL output which is strange. Will look into what their code when I’ll have the time

[MansMeg]

Sure! I can review. @kdubovikov if tou check the failing error as a first syep that is great. I might be a little too defensive in the test suite, so that might be the reason.

[kdubovikov]

Tests were failing due to debug prints that I forgot to delete. Everything should be working well now. By the way, @MansMeg , an impressive test set 👍

[MansMeg]

Great! I will start to review it asap.

[MansMeg]

I think this looks good. The only open question now is the documentation without a function.

A question is also if we can add a test to check that this works with cores > 1? I'm not sure this is a part of the test suite.

[codecov-commenter]

Codecov Report

Merging #171 (cd60a8e) into master (ca3e225) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #171   +/-   ##
=======================================
  Coverage   95.15%   95.15%           
=======================================
  Files          28       28           
  Lines        2662     2664    +2     
=======================================
+ Hits         2533     2535    +2     
  Misses        129      129           

Impacted Files	Coverage Δ
R/loo_subsample.R	95.30% <100.00%> (+<0.01%)	⬆️
R/E_loo.R	98.86% <0.00%> (ø)
R/loo_compare.R	94.79% <0.00%> (ø)
R/loo_moment_matching.R	99.21% <0.00%> (+<0.01%)	⬆️

---

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 ca3e225...cd60a8e. Read the comment docs.

[MansMeg]

Alright. It seems that we do not have any tests for testing that multiple cores are working as expected. @jgabry do you have any way of testing multicore in any of the test suites? The additional part this is currently not tested are lines 495:503. It looks good to me, but I guess we would like to test multicore performance in the CI?

[kdubovikov]

Alright. It seems that we do not have any tests for testing that multiple cores are working as expected. @jgabry do you have any way of testing multicore in any of the test suites? The additional part this is currently not tested are lines 495:503. It looks good to me, but I guess we would like to test multicore performance in the CI?

@MansMeg I have added patrick package and parameterized your elpd_loo_approximation test so that it would check multiple cores as a separate case. Do not know if you have multicore containers in CI though

[MansMeg]

I think we should avoid adding dependencies to the loo package as much as possible. Let's see what @jgabry say. We should conform to the way multicore stuff is tested currently in the package. The patrick package is version 0.0.3 and hence very unmature right now - so I would discourage using it altogether.

[jgabry]

patrick looks like a useful package but I agree with @MansMeg that we should avoid adding it as a dependency if possible.

It's ok to use up to 2 cores in tests on CRAN I believe. We don't have tests checking multicore speed, but we do have tests in other files that check whether the parallelized version and the non-parallelized version return the same answer. For example:

loo/tests/testthat/test_loo_and_waic.R

Lines 32 to 35 in 0117858

	test_that("loo with cores=1 and cores=2 gives same results", {
	loo2 <- suppressWarnings(loo(LLarr, r_eff = r_eff_arr, cores = 2))
	expect_equal(loo1$estimates, loo2$estimates)
	})

The loo1 object in that test was created with cores=1 and loo2 with cores = 2.

If you do want to test that multicore is faster then comparing system.time() would probably be sufficient.

[MansMeg]

I think this definitely is sufficient, the main thing is that we want to check that we get the same reault with and without multicore on win and mac machines. I guess this will now be checked if we go with this solution?

@kdubovikov is it ok for you to go with this solution instead of patrick?

[MansMeg]

Ping @kdubovikov

[kdubovikov]

@MansMeg sorry, was occupied by work. Sure, it is ok for me to do it. I will update the code as soon as I will have the time

[MansMeg]

Great! I think it would be very nice to get parallelism into the subsampling functionality.

[kdubovikov]

@MansMeg done, simplified tests and removed the dependency

[MansMeg]

I have now check this and I think it looks good! I'm happy to add this functionality and it makes a lot of sense. Although, I think it is best that you @jgabry do the merge to master.

[MansMeg]

Although, there seems to be an error in the run of the code coverage in github actions.

[MansMeg]

I think this looks good, except that there seem to be some problem with the code coverage github action code.

[jgabry]

Forgot that this hadn't been merged yet! Merging now.