Closed n-kall closed 10 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
0322b46
) 95.14% compared to head (4abb2fb
) 95.15%.:exclamation: Current head 4abb2fb differs from pull request most recent head 3c3d7e8. Consider uploading reports for the commit 3c3d7e8 to get more accurate results
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
This is how benchmark results would change (along with a 95% confidence interval in relative change) if c25f45291c2d0c1440ba379855cf429e9d4bc9e4 is merged into master:
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 976b950edb1bf7a13acbe358827e84fcfc20eab0 is merged into master:
@avehtari Should it be ceiling() or round() on the thinning value if non-integer? In the issue #127 it says ceiling, but I don't remember what we decided
How about thinning with non-integers? E.g. round(seq(1,100,by=max(1,1.5)))
gives valid indeces
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 126d6dc21ff473ab00f08e94fdb3fcca6c1c2e89 is merged into master:
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 98dea553a85c498c932dfbc94959689bd8c950f8 is merged into master:
This is how benchmark results would change (along with a 95% confidence interval in relative change) if b32dfe8d111e1473aa7a667e271ac169b5ae9aca is merged into master:
This is how benchmark results would change (along with a 95% confidence interval in relative change) if c5c1db02df76d7eb3bb27fb131ddff73ecc22ec2 is merged into master:
I've changed the thinning to handle non-integer values. I think this is now ready for further review
Looks good to me. @avehtari do you have any comments? Otherwise, I will merge.
It would be good that the documentation would explicitly mention what happens with non-integer thin values. Even if people would not use non-integer as argument, if they use automatic thinning, they may be surprised by the number of returned iterations unless behavior of non-integer thin is documented
@avehtari how about this for the doc:
"thin (numeric) The period for selecting draws. Must be between 1 and the number of iterations. If the value is not an integer, the draws will be selected such that the average interval between them approaches the thin value. If NULL
, it will be automatically calculated based on bulk and tail effective sample size as suggested by Säilynoja et al. (2022)."
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 4abb2fbb1c39cc7e436efddd70c487e4ac37c124 is merged into master:
This is how benchmark results would change (along with a 95% confidence interval in relative change) if a8b50b58ee14afc6cd01b0bb9ae0f0e1e3c78061 is merged into master:
Updated the documentation based on dicussions with @avehtari. I think it's ready now
OK for me
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 6555ac87c5fc6ab348e50adba4e797bf95baf045 is merged into master:
thanks!
Summary
After discussions with @avehtari this PR adds automatic thinning of draws based on ESS as suggested by Säilynoja et al. 2022 (appendix).
Thin by = S / (min(ess_tail, ess_bulk)). For multiple chains, this is ndraws_per_chain / mean_over_chains(min(ess_tail, ess_bulk))
Addresses issue #127
Copyright and Licensing
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: