dynesty icon indicating copy to clipboard operation
dynesty copied to clipboard

Add additional rwalk proposals

Open ColmTalbot opened this issue 2 years ago • 3 comments

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

ColmTalbot avatar Apr 07 '22 00:04 ColmTalbot

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%
<!-- Total: 144 146
Totals Coverage Status
Change from base Build 2157424611: 0.2%
Covered Lines: 3767
Relevant Lines: 4169

💛 - Coveralls

coveralls avatar Apr 07 '22 16:04 coveralls

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 avatar Apr 07 '22 17:04 segasai

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 ?

segasai avatar Apr 19 '22 13:04 segasai