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

Adjusts for the fact that IRMH proposal might not be symmetric #581

Merged
merged 5 commits into from
Oct 31, 2023

Conversation

ciguaran
Copy link
Contributor

Thank you for opening a PR!

Adds proposal_logdensity_fn as parameter, so as to modify MH acceptance rule to accommodate for non-symmetry of the proposal distribution.

A few important guidelines and requirements before we can merge your PR:

  • [N] If I add a new sampler, there is an issue discussing it already;
  • [Y] We should be able to understand what the PR does from its title only;
  • [Y] There is a high-level description of the changes;
  • [N/A] There are links to all the relevant issues, discussions and PRs;
  • [Y] The branch is rebased on the latest main commit;
  • [Y] Commit messages follow these guidelines;
  • [Y] The code respects the current naming conventions;
  • [Y] Docstrings follow the numpy style guide
  • [Y] pre-commit is installed and configured on your machine, and you ran it before opening the PR;
  • [N] There are tests covering the changes;
    I haven't added new tests since this is just calling build_rmh from within a factory (IRMH is indeed a factory) and the functionality is already well tested.
  • [Y] The doc is up-to-date;
  • [N] If I add a new sampler* I added/updated related examples

Consider opening a Draft PR if your work is still in progress but you would like some feedback from other contributors.

Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Merging #581 (9138a86) into main (f5c0822) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #581   +/-   ##
=======================================
  Coverage   99.14%   99.14%           
=======================================
  Files          49       49           
  Lines        2117     2117           
=======================================
  Hits         2099     2099           
  Misses         18       18           
Files Coverage Δ
blackjax/mcmc/random_walk.py 100.00% <100.00%> (ø)

@junpenglao
Copy link
Member

Thanks, could you add a small test?

blackjax/mcmc/random_walk.py Outdated Show resolved Hide resolved
blackjax/mcmc/random_walk.py Outdated Show resolved Hide resolved
"""

def proposal_generator(rng_key: PRNGKey, position: ArrayTree):
del position
Copy link
Member

Choose a reason for hiding this comment

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

reverse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, just out of curiosity why is that del needed?

Copy link
Member

Choose a reason for hiding this comment

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

It's not needed, mostly it is styling to signify the input arg is not needed.

@ciguaran
Copy link
Contributor Author

ciguaran commented Oct 31, 2023

Thanks, could you add a small test?

Added the smallest test that only tests the behavior within this kernel, so as to avoid retesting MH acceptance rule, which is already tested in another place.

@junpenglao junpenglao merged commit f5a2a12 into blackjax-devs:main Oct 31, 2023
7 checks passed
junpenglao pushed a commit that referenced this pull request Mar 12, 2024
* Adjusting for the fact that the IRMH proposal might not be symmetric

* Fixing type annotation

* Adding proposal_logdensity_fn to the interface exposed to users, with default parameter

* Applying pre-commit to all files

* Adding minimal test
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