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

Align DEMetropolis defaults with literature recommendations #6488

Merged
merged 4 commits into from
Feb 1, 2023

Conversation

gbrunkhorst
Copy link
Contributor

What is this PR about?
Changing the defaults for DEMetropolis and DEMetropolisZ samplers to proposal_dist=pm.NormalProposal and tune='scaling' as discussed with @michaelosthege here. This is for consistency with the approach in terBraak2008 paper (which discussed using a proposal distribution with unbounded support such as the Normal distribution).

The changes consist of two lines of code plus docstrings for each sampler.

Tests. I followed the instructions here and had to update the test_metropolis.py file to match the new defaults.

Major / Breaking Changes

  • Since the default assumptions are changed code that uses defaults would be slightly different. However, this should be an improvement.
  • Specifying the arguments tune and proposal_dist allows one to revert to the previous settings.

New features

  • no

Bugfixes

  • no

Documentation

  • documentation has been updated consistent with modifications

Maintenance

  • no

Copy link
Member

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

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

Just adding a few small changes to fix pre-commit (black would complain about ' strings) and aligning the docstrings

pymc/step_methods/metropolis.py Outdated Show resolved Hide resolved
pymc/step_methods/metropolis.py Outdated Show resolved Hide resolved
pymc/step_methods/metropolis.py Outdated Show resolved Hide resolved
pymc/step_methods/metropolis.py Outdated Show resolved Hide resolved
pymc/step_methods/metropolis.py Outdated Show resolved Hide resolved
pymc/step_methods/metropolis.py Outdated Show resolved Hide resolved
@michaelosthege michaelosthege added the major Include in major changes release notes section label Jan 28, 2023
@codecov
Copy link

codecov bot commented Jan 28, 2023

Codecov Report

Merging #6488 (7bbd7f4) into main (902b1ec) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6488      +/-   ##
==========================================
- Coverage   94.79%   94.79%   -0.01%     
==========================================
  Files         148      148              
  Lines       27732    27734       +2     
==========================================
+ Hits        26289    26290       +1     
- Misses       1443     1444       +1     
Impacted Files Coverage Δ
pymc/step_methods/metropolis.py 83.58% <100.00%> (-0.22%) ⬇️
pymc/tests/step_methods/test_metropolis.py 100.00% <100.00%> (ø)

@michaelosthege michaelosthege changed the title Demetropolis defaults Align DEMetropolis defaults with literature recommendations Feb 1, 2023
@michaelosthege michaelosthege merged commit af8cab2 into pymc-devs:main Feb 1, 2023
@michaelosthege
Copy link
Member

Thanks @gbrunkhorst for the thorough analysis in the other PR, and for updating the defaults here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major Include in major changes release notes section
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants