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

Add additional rwalk proposals #366

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ColmTalbot
Copy link
Contributor

This PR adds a bunch of different methods for proposing points with the rwalk sampling.

These are a combination of different scale distributions for the existing method and ensemble moves.

  • The default behaviour is different, I'm happy to change that to preserve the defaults if desired.
  • All methods are tested in the unit tests.

Some of the unit tests are failing, I still need to dig into that.

cc @joshspeagle @segasai

@coveralls
Copy link

coveralls commented Apr 7, 2022

Pull Request Test Coverage Report for Build 2173142774

  • 144 of 146 (98.63%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 90.357%

Changes Missing Coverage Covered Lines Changed/Added Lines %
py/dynesty/proposals.py 73 74 98.65%
py/dynesty/sampling.py 45 46 97.83%
Totals Coverage Status
Change from base Build 2157424611: 0.2%
Covered Lines: 3767
Relevant Lines: 4169

💛 - Coveralls

@segasai
Copy link
Collaborator

segasai commented Apr 7, 2022

Thanks for the PR.

In my opinion I would prefer to start merging just the ensenble sampling part and leave outside the axis based proposals and chi-proposals (unless there is a clear demonstrable benefit).
Also I think we have to be careful with the naming in the patch 'proposals', 'ellipsoidal_proposals' to avoid confusion with uniform ellipsoidal sampling etc. (at the moment there are also various names used by the existing sampler:
we start by proposing the point (usually one of the live points), we then evolve the point; the last bit is done by various slice/walk samplers; I think some of those names are not great, but should try to make sure that things are consistently named so we don't confuse ourselves)

For the ensemble sampling part, it seems that there is an easy to write example of a multimodal distribution where it'd be beneficial. I think it'd be good to put that example here, so we can assess the improvement/test that it perfoms like it should. (IMO the test suite is mostly checking that things can run and are not outright broken, rather testing the efficiency)

@segasai
Copy link
Collaborator

segasai commented Apr 19, 2022

I have spent some time looking and thinking about this proposal, and my current thinking (on top of what I said previously) is that

  • we should keep the existing rwalk as it is now
  • Introduce a new 'pluggable' sampling interface, that can do sampling of one nested sampling step.
    i.e. so we can do things like
    dynesty.Nestedsampler(sample=dynesty.sampler.EnsembleSampler(a=3,b=4))
    dynesty.Nestedsampler(sample=dynesty.sampler.RWalkSampler(walks=44))
  • Those samplers would exactly implement the changes from this and neighboring PR.

We would need some kind of interface for those samplers.
Right now the samplers essentially have propose_point, evolve_point methods, but that's probably
too restrictive for example for the ensemble sampler. In fact it's unclear if we want to keep the separation between propose and evolve.

As far I can see the classes defined here
https://github.com/joshspeagle/dynesty/blob/master/py/dynesty/nestedsamplers.py
that are boundary specific will need to take as an argument classes that will need to do sampling operations like:
Take the list of points subject to L>Lconstraint, (optional boundary specification) -> propose a new point (or several) subject to constraint (L>L_)

as far as I can see that would allow us more clearly add new samplers (and have them implemented externally even), and avoid adding more options for NestedSampler.

Thoughts ?

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