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

Refactored MLDA proposal to not use trace continuation #5095

Merged
merged 4 commits into from
Oct 23, 2021

Conversation

mikkelbue
Copy link
Contributor

This PR removes the reliance on trace continuation for the MLDA proposal, as described in #5021.

A few other updates to mlda.py and test_step.py are also included:

  • Removing reliance on trace continuation by using a new trace for each subchain.
  • Fixed bug in reversed MLDA use of DictToArrayBijection that sometimes flipped the order of variables.
  • Making the MLDA-specific subsample function more compact and concise.
  • Use compile_rv_inplace in place of aesara.function in the delta_logp_inverse function.
  • Added typing to MDLA core methods and functions.
  • Rename value_vars to vars to harmonise with other PyMC step methods.
  • Reintroduce MLDA tests in test_step.py.
  • Fix flaky test in test_step.py::TestMLDA::test_variance_reduction. This test no longer asserts whether the variance is actually reduced since this cannot be guaranteed for any seed. It only checks that the functionality is working and the correct types are returned.

@codecov
Copy link

codecov bot commented Oct 22, 2021

Codecov Report

Merging #5095 (6bebc0e) into main (f26845d) will increase coverage by 1.38%.
The diff coverage is 94.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5095      +/-   ##
==========================================
+ Coverage   76.28%   77.66%   +1.38%     
==========================================
  Files          87       87              
  Lines       14107    14099       -8     
==========================================
+ Hits        10761    10950     +189     
+ Misses       3346     3149     -197     
Impacted Files Coverage Δ
pymc/step_methods/mlda.py 96.05% <94.87%> (+48.37%) ⬆️
pymc/step_methods/metropolis.py 83.70% <0.00%> (+0.22%) ⬆️
pymc/tests/models.py 87.76% <0.00%> (+5.75%) ⬆️

Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

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

From a quick glance it looks good to me!

@twiecki twiecki merged commit 3ab1c00 into pymc-devs:main Oct 23, 2021
@twiecki
Copy link
Member

twiecki commented Oct 23, 2021

Thanks @mikkelbue!

@twiecki
Copy link
Member

twiecki commented Oct 25, 2021

@mikkelbue #5099

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.

3 participants