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

Check only for negative divergences #574

Merged

Conversation

jdehning
Copy link
Contributor

@jdehning jdehning commented Oct 19, 2023

Currently, a divergence error is triggered both when the energy increases and decreases excessively. However a large delta_energy can also occur in complex models, when initial points are widely outside the typical set. Then proposal steps typically vastly improve the likelihood and have large energy differences, but these are currently rejected. Changing the current behavior to only trigger when delta_energy is excessively negative fixes this behavior.

This is in line with the implementation in Stan: https://github.com/stan-dev/stan/blob/88cffcf6271da06b0474dd6ce90d69a54127a5e6/src/stan/mcmc/hmc/nuts/base_nuts.hpp#L262C11-L262C19 and Numpyro: https://github.com/pyro-ppl/numpyro/blob/065aa4352dbc9ebeb1132b15b51d8b3149848640/numpyro/infer/hmc.py#L382C13-L382C13

I only modified the documentation of the local function, otherwise left that divergence_threshold is the "max value allowed for the difference in energies to be considered a divergence". I think it would lead to more confusion than be helpful if replaced by "an increase". Also, effectively, when the chain converged to the typical set, it shouldn't change anything.

  • If I add a new sampler, there is an issue discussing it already;
  • We should be able to understand what the PR does from its title only;
  • There is a high-level description of the changes;
  • There are links to all the relevant issues, discussions and PRs;
  • The branch is rebased on the latest main commit;
  • Commit messages follow these guidelines;
  • The code respects the current naming conventions;
  • Docstrings follow the numpy style guide
  • pre-commit is installed and configured on your machine, and you ran it before opening the PR;
  • There are tests covering the changes;
  • The doc is up-to-date;
  • If I add a new sampler* I added/updated related examples

Change the behavior of the divergence check to
only check whether the negative energy difference
is larger than the maximally allowed difference.
This allows proposals from widely out-of-typical-set
initial points to not trigger the divergence threshold.
@junpenglao
Copy link
Member

Thanks!

Currently, a divergence error is triggered both when the energy increases and decreases excessively. However a large delta_energy can also occur in complex models, when initial points are widely outside the typical set. Then proposal steps typically vastly improve the likelihood and have large energy differences, but these are currently rejected. Changing the current behavior to only trigger when delta_energy is excessively negative fixes this behavior.

Yes this make sense. Usually divergence error was check after warm up, where the sampler should be already in the typical set thus large energy change in either direction is unlikely.

@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

Merging #574 (385060a) into main (51cf08a) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #574      +/-   ##
==========================================
- Coverage   99.14%   99.14%   -0.01%     
==========================================
  Files          49       49              
  Lines        2099     2098       -1     
==========================================
- Hits         2081     2080       -1     
  Misses         18       18              
Files Coverage Δ
blackjax/mcmc/proposal.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@junpenglao junpenglao merged commit 5fdd603 into blackjax-devs:main Oct 19, 2023
6 of 7 checks passed
@jdehning jdehning deleted the divergence_check_modification branch October 20, 2023 09:37
junpenglao pushed a commit that referenced this pull request Mar 12, 2024
Change the behavior of the divergence check to
only check whether the negative energy difference
is larger than the maximally allowed difference.
This allows proposals from widely out-of-typical-set
initial points to not trigger the divergence threshold.
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.

2 participants