Update ess_hat to not error for short chains #119
Conversation
Pull Request Test Coverage Report for Build 7900491678Details
💛 - Coveralls |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #119 +/- ##
==========================================
+ Coverage 96.65% 96.68% +0.03%
==========================================
Files 11 11
Lines 867 876 +9
==========================================
+ Hits 838 847 +9
Misses 29 29 ☔ View full report in Codecov by Sentry. |
|
The remaining error on the MCMCChains integration test is expected. |
src/ess_rhat.jl
Outdated
| @@ -94,7 +94,7 @@ function build_cache(::FFTAutocovMethod, samples::Matrix, var::Vector) | |||
|
|
|||
| # create cache for FFT | |||
| T = complex(eltype(samples)) | |||
| n = nextprod([2, 3], 2 * niter - 1) | |||
| n = nextprod([2, 3], 2 * max(1, niter) - 1) | |||
There was a problem hiding this comment.
Since we skip computation of ESS in the special case niter <= 4 anyway, wouldn't it be easier to also skip computing esscache and rel_ess_max? Possibly it's still nice/an improvement to handle the case niter < 1 more gracefully here.
There was a problem hiding this comment.
I refactored the code to separate allocation of outputs from the allocation of caches with computation of values. When number of iterations is too low, this allows us to directly call the mutating rhat function, which requires no caches.
|
@devmotion it seems I don't have permissions to merge even with approval because the required integration test doesn't pass (expected). Could my role be upgraded to Admin? |
|
@sethaxen I made you an admin and changed the settings such that admins may bypass the branch protection requirements. |
|
Thanks! |
Fixes #117. As a side benefit,
ess_rhatfor therhatnow returns the same thing asrhateven for few draws.