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

more robust U-turn checking #145

Merged
merged 3 commits into from
Feb 11, 2021
Merged

more robust U-turn checking #145

merged 3 commits into from
Feb 11, 2021

Conversation

tpapp
Copy link
Owner

@tpapp tpapp commented Jan 15, 2021

  • add a test demonstrating this (based on MWE by @sethaxen)
  • modify algorithm

Fixes #115. See discussion at Stan forum.

@tpapp tpapp changed the title more robust U-turn checking WIP: more robust U-turn checking Jan 15, 2021
@tpapp tpapp changed the title WIP: more robust U-turn checking more robust U-turn checking Feb 11, 2021
@tpapp tpapp merged commit 1d24105 into master Feb 11, 2021
@tpapp tpapp deleted the tp/robust-u-turn branch February 12, 2021 14:59
facebook-github-bot pushed a commit to facebookresearch/beanmachine that referenced this pull request Jun 4, 2021
Summary:
Pull Request resolved: #864

An issue with the U-turn condition was discovered and discussed in [this post in Stan forum](https://discourse.mc-stan.org/t/nuts-misses-u-turns-runs-in-circles-until-max-treedepth/9727)

TL;DR: we can make the U-turn condition more robust by introducing two additional checks across subtrees. This can help us avoid missing U-turns for approximately iid normal models.

{F619223264}

Since the tree combining code are almost identical in `_build_tree` and `propose`, I also take the chance to refactor them into a common function called `_combine_tree`. If you look closely you will notice that most part of `_combine_tree` are moved from existing code as-is. The only addition is the two additional call to `_is_u_turning`

Related PR that implements this change:
- Stan: stan-dev/stan#2800
- PyMC3: pymc-devs/pymc#3605
- Turing.jl: TuringLang/AdvancedHMC.jl#207
- DynamicHMC.jl: tpapp/DynamicHMC.jl#145

Reviewed By: neerajprad

Differential Revision: D28735950

fbshipit-source-id: ada4ebcad26a87ef5e697f422b5c5b17007afe42
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.

Implement fix for NUTS missing u-turns
1 participant