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

Improve ESS estimates for antithetic chains #58

Merged
merged 22 commits into from
Jan 16, 2023
Merged

Improve ESS estimates for antithetic chains #58

merged 22 commits into from
Jan 16, 2023

Conversation

sethaxen
Copy link
Member

@sethaxen sethaxen commented Jan 9, 2023

This PR fixes #40. It's marked as draft currently because I plan to use the test utilities in #53 to write the tests.

@coveralls
Copy link

coveralls commented Jan 9, 2023

Pull Request Test Coverage Report for Build 3911141108

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 95.225%

Totals Coverage Status
Change from base Build 3906657106: 0.01%
Covered Lines: 678
Relevant Lines: 712

💛 - Coveralls

@codecov
Copy link

codecov bot commented Jan 9, 2023

Codecov Report

Base: 95.63% // Head: 95.64% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (50c919e) compared to base (08f359c).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 50c919e differs from pull request most recent head 83b1ca3. Consider uploading reports for the commit 83b1ca3 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #58      +/-   ##
==========================================
+ Coverage   95.63%   95.64%   +0.01%     
==========================================
  Files          10       10              
  Lines         710      712       +2     
==========================================
+ Hits          679      681       +2     
  Misses         31       31              
Impacted Files Coverage Δ
src/ess.jl 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

src/ess.jl Outdated Show resolved Hide resolved
Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

Thank you! A preliminary comment: could you add some references (to eg our discussion and in particular your last comment and the references therein) such that it is clear why these changes are added and where eg the bound on ESS came from?

src/ess.jl Outdated Show resolved Hide resolved
@sethaxen sethaxen marked this pull request as ready for review January 13, 2023 19:24
@sethaxen sethaxen requested a review from devmotion January 13, 2023 19:24
@sethaxen sethaxen closed this Jan 16, 2023
@sethaxen sethaxen reopened this Jan 16, 2023
@sethaxen
Copy link
Member Author

@devmotion this is ready for final review

src/ess.jl Outdated Show resolved Hide resolved
src/ess.jl Outdated Show resolved Hide resolved
src/ess.jl Outdated Show resolved Hide resolved
# - https://github.com/TuringLang/MCMCDiagnosticTools.jl/issues/40
# - https://github.com/stan-dev/rstan/pull/618
# - https://github.com/stan-dev/stan/pull/2774
τ = max(0, 2 * sum_pₜ + max(0, ρ_even) - 1)
Copy link
Member

Choose a reason for hiding this comment

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

Is this line "correct", i.e., desired, even if we never run the while loop (i.e., if maxlag <= 2? In these cases it would simplify to τ = max(0, 2 * sum_pₜ) since ρ_even = 1. And in contrast to the other values we would use the last ρ_odd.

Copy link
Member Author

Choose a reason for hiding this comment

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

No it's not. I've modified to compute the final ρ_even only if maxlag >= 2. Otherwise it is set to 0 before this line.

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

Thank you, looks good to me!

@sethaxen sethaxen merged commit 1799e79 into main Jan 16, 2023
@delete-merged-branch delete-merged-branch bot deleted the fixnegess branch January 16, 2023 21:41
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.

Negative ESS
3 participants