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

Implement robust U-turn check #3605

Merged
merged 18 commits into from
Oct 17, 2019
Merged

Implement robust U-turn check #3605

merged 18 commits into from
Oct 17, 2019

Conversation

junpenglao
Copy link
Member

@junpenglao junpenglao commented Aug 24, 2019

Following the recent discussion on the Stan side: stan-dev/stan#2800

@junpenglao
Copy link
Member Author

Thanks to @aseyboldt's design, adding additional U turn check to pymc3 NUTS is actually pretty straightforward, because we are already saving the beginning and the end point of each tree trajectory.

fix error in recording the end point of the reversed subtree
Following the recent discussion on the Stan side: stan-dev/stan#2800

For experiment, do not merge.
fix error in recording the end point of the reversed subtree
@twiecki
Copy link
Member

twiecki commented Aug 30, 2019

Looks like this needs a rebase.

@junpenglao junpenglao requested a review from aseyboldt October 17, 2019 09:20
@codecov
Copy link

codecov bot commented Oct 17, 2019

Codecov Report

Merging #3605 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3605      +/-   ##
==========================================
+ Coverage   89.77%   89.79%   +0.02%     
==========================================
  Files         134      134              
  Lines       20027    20047      +20     
==========================================
+ Hits        17980    18002      +22     
+ Misses       2047     2045       -2
Impacted Files Coverage Δ
pymc3/step_methods/hmc/nuts.py 99.35% <100%> (+0.09%) ⬆️
pymc3/step_methods/hmc/base_hmc.py 96.26% <0%> (+1.86%) ⬆️

@aseyboldt
Copy link
Member

This looks good to me, I'd be in favor of merging.

@junpenglao junpenglao changed the title [WIP] Robust U-turn check Implement robust U-turn check Oct 17, 2019
@ColCarroll
Copy link
Member

+1 I'll also leave this here for us to keep an eye on :D

https://pandas.pydata.org/speed/pymc3/

@twiecki twiecki merged commit a01d015 into pymc-devs:master Oct 17, 2019
@twiecki
Copy link
Member

twiecki commented Oct 17, 2019

👍

@junpenglao junpenglao deleted the robust_u_turn branch October 17, 2019 16:43
@ColCarroll ColCarroll mentioned this pull request Nov 27, 2019
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.

4 participants