Use testthat 3e and rely on posterior for ESS

[VisruthSK]

One test was failing due to minor numerical differences in algorithms: average diff: 0.00975, largest diff: 0.01472. I updated the expected test result at reference-results/relative_eff.rds.

posterior::autovariance is already used.

Starts work on #249

[jgabry]

Seems like it should be fine to make this change. There are additional tests failing due to differences with the saved reference objects. They seem small, but would be good for @avehtari to confirm that these differences are reasonable to expect due to (I assume) minor differences between this older ESS code and the ESS algorithm used in posterior.

[avehtari]

Differences in ESS with magnitude in order of 1e-2 are small and can be ignored.

Instead of posterior::ess_basic(), I would use posterior::ess_mean() as the name makes it more explicit what is computed. They use the same approach (but ess_basic() allows also additional argument which can be used to turn off the splitting).

[VisruthSK]

Is there a neat way to update all expected outputs to the new values so that tests pass?

[jgabry]

I think if we updated to using the newer testthat::expect_snapshot() then there's a way to update all of them at one. The older expect_equal_to_reference() doesn't seem to have that ability unfortunately. At some point we should update to snapshot testing (it wasn't part of testthat when I wrote these tests a long time ago).

If you want to do that as part of this PR we could, but also fine to hold off on that and just update the reference files.

[jgabry]

If you don't want to update to snapshot testing then I think the east way might be to just delete the out of date reference files and then run the tests locally on your local machine and it should generate new files. But I think you need to run the tests interactively for it to generate the new files. That is, you can't just do devtools::test(), you need to open the relevant test file and run all the tests (you can run all of them, you don't need to do it one by one).

[VisruthSK]

I think swapping to snapshots is worthwhile, so I'll get started on that. It'll make this PR easier so might as well include it here. I briefly tried updating the reference files but I wasn't able to get the reference files to update easily.

[avehtari]

Looks good to me.

I assume you update only the snapshots if the differences were small? And in case of big differences, investigate the reason first?

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.78%. Comparing base (b94b2b1) to head (c3107f0).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
R/effective_sample_sizes.R	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #289      +/-   ##
==========================================
+ Coverage   92.54%   92.78%   +0.23%     
==========================================
  Files          31       31              
  Lines        3017     2964      -53     
==========================================
- Hits         2792     2750      -42     
+ Misses        225      214      -11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jgabry]

Thanks! Is this ready for me to take another look or are you still working on this? Either way is fine, just checking.

[VisruthSK]

Yup it should be ready for a new review, sorry for not requesting.

There is one thing--I haven't updated the relative_eff with multiple cores runs test and don't understand how it is currently set up. That test is failing due to the move to posterior, and I want to change the test but I'm not sure how to exactly.

[jgabry]

There is one thing--I haven't updated the relative_eff with multiple cores runs test and don't understand how it is currently set up. That test is failing due to the move to posterior, and I want to change the test but I'm not sure how to exactly.

When I switch to your branch and run that test it passes for me. It also seems to be passing on GitHub Actions, right? It's failing for you locally?

We're talking about this test, right?

test_that("relative_eff with multiple cores runs", {
  skip_on_cran()
  source(test_path("data-for-tests/function_method_stuff.R"))
  dim(llmat_from_fn) <- c(nrow(llmat_from_fn), 1, ncol(llmat_from_fn))
  r_eff_arr <- relative_eff(llmat_from_fn, cores = 2)
  r_eff_fn <-
    relative_eff(
      llfun,
      chain_id = rep(1, nrow(draws)),
      data = data,
      draws = draws,
      cores = 2
    )
  expect_identical(r_eff_arr, r_eff_fn)
})

[VisruthSK]

Yes it's failing locally. I thought the action might have skipped no Cran tests so it passed, but if it worked locally for you that's interesting. Maybe I have the wrong version of posterior?

Edit: for future reference, the test was failing because I was running that specific file test_active_file() which loaded the loo package at the start. I didn't build and install the development version of loo so the test wouldn't pass. Running check() or installing the local version of the package solved the issue.

[jgabry]

I think this looks good. Will merge now!