Closed segasai closed 3 years ago
On this subject, I'm also proposing making rslice as a default sampler in high dimensions, rather than using rwalk in 10..20 dimensions range.
Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
---|---|---|---|---|---|
py/dynesty/dynesty.py | 19 | 24 | 79.17% | ||
<!-- | Total: | 19 | 24 | 79.17% | --> |
Files with Coverage Reduction | New Missed Lines | % | ||
---|---|---|---|---|
py/dynesty/dynesty.py | 1 | 77.51% | ||
py/dynesty/sampling.py | 6 | 91.4% | ||
<!-- | Total: | 7 | --> |
Totals | |
---|---|
Change from base Build 951288534: | -0.0008% |
Covered Lines: | 3693 |
Relevant Lines: | 4596 |
I'm also proposing making rslice as a default sampler in high dimensions, rather than using rwalk
Is that just because it gets it is more robust, or also that at this point its performance is actually better as well?
I think rslice is quite robust now. It also requires less steps than rwalk. I think rwalk also occasionally produce the 'random walk is very inefficient' messages and is likely more sensitive to bad scaling. I have a strong opinion that rslice is now more stable, but I don't have hard numbers. (for my own stuff, I now almost always use rslice)
Each step for rslice
is multiple function calls though, which is why I was curious what the overall sampling efficiency looked like by comparison. If it's not a huge efficiency hit in that dimension range while being more robust overall, I agree that the defaults can be changed.
Here's the Gaussian test.
Unexpectedly (to me) rwalk indeed outperforms rslice (roughty factor of 3 in number of calls). So I temporarily retract the change of the default sampler for 10-20 ndims. I'm still wondering if there is something not quite right with scaling up or down the step size for slice sampling (as that seemed to misbehave occasionally)
That adaptive step was supposed to make sampling more efficient overall based on what I had seen in the literature and from other tests (like with rwalk
), but if it looks like it hurts more than it helps I'm happy to disable it by default.
I don't have a hard proof (ATM), so lets not change anything without it, but lets keep it in mind.
I tried to merge this into the main branch, but I think I slightly screwed up the line editing -- sorry. Should be a quick fix on your end whenever you have a moment; otherwise, I can get to this Monday.
Fixed it. The problem is that the live_points argument for some reason only is accepted by the nestedsampler not dynamic one. (and previously the test just ran the test ignoring the live_points arg, but after #295 the code started to complain)
address #289