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

Add accepted arg to AdvancedMH.Transition calls #2172

Merged
merged 2 commits into from Feb 16, 2024

Conversation

sunxd3
Copy link
Collaborator

@sunxd3 sunxd3 commented Feb 16, 2024

@devmotion
Copy link
Member

As I wrote in TuringLang/AdvancedMH.jl#88 this approach can easily lead to misleading/incorrect acceptance rates?

@sunxd3
Copy link
Collaborator Author

sunxd3 commented Feb 16, 2024

The field accepted is added, but seemingly not used anywhere right now. I can see it causing issue particularly with thinning, but maybe we can always return the acceptance rate of the un-thin-ed chain? I am certain here

@sunxd3
Copy link
Collaborator Author

sunxd3 commented Feb 16, 2024

Anyhow, this PR is to just patch the interface so that it won't error.

Copy link

codecov bot commented Feb 16, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (990eeb2) 0.00% compared to head (6d2f291) 0.00%.
Report is 1 commits behind head on master.

Files Patch % Lines
src/mcmc/mh.jl 0.00% 2 Missing ⚠️
src/mcmc/emcee.jl 0.00% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##           master   #2172   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          22      22           
  Lines        1385    1385           
======================================
  Misses       1385    1385           

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

@yebai
Copy link
Member

yebai commented Feb 16, 2024

As I wrote in TuringLang/AdvancedMH.jl#88 this approach can easily lead to misleading/incorrect acceptance rates?

Can we print a warning message indicating that the acceptance rate might be inaccurate if thinning or discarding warmup is applied?

test_code.jl Outdated
@@ -0,0 +1,126 @@
using Turing, Enzyme, LogDensityProblems, LogDensityProblemsAD
Copy link
Member

Choose a reason for hiding this comment

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

Better to use a more informative file name here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, sorry, this file should not be here, it was a local script, I'll remove it

@yebai yebai merged commit cf647b1 into master Feb 16, 2024
13 checks passed
@yebai yebai deleted the sunxd/add_accepted_to_AMH_transition branch February 16, 2024 19:08
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.

None yet

3 participants