Parallel uniform sampling + New sampler interface by segasai · Pull Request #471 · joshspeagle/dynesty

segasai · 2024-02-14T17:17:02Z

This is a preliminary version of patch that enables parallel uniform sampling.
The idea is to get away from this propose/evolve for uniform distributions where evolve doesn't really do anything other than evaluate logl on a single pt, but instead make propose a no-op for uniform, and evolve() actually do the sampling inside the boundary till a satisfactory pt is found.

While implementing this I realized that after some changes all the different samplers in nestedsamplers.py may not need to be there as they do something very similar (but that's tbd).

ATM some tests fail, but I think that's fixable.

Also some class members had to be renamed. I.e. .bound attribute is really misleading in the classes as it was storing the list of all the historic bounds, so I renamed that to bound_list.

nestedsamplers classes can probably be removed at some point

coveralls · 2024-02-14T20:38:15Z

Pull Request Test Coverage Report for Build 7952108689

Details

-12 of 225 (94.67%) changed or added relevant lines in 6 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.2%) to 91.863%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
py/dynesty/sampling.py	28	30	93.33%
py/dynesty/bounding.py	30	34	88.24%
py/dynesty/sampler.py	136	142	95.77%

Files with Coverage Reduction	New Missed Lines	%
py/dynesty/bounding.py	1	92.24%

Totals
Change from base Build 7180723466:	0.2%
Covered Lines:	4053
Relevant Lines:	4412

💛 - Coveralls

joshspeagle · 2024-02-14T22:01:11Z

!!!! This is an excellent proposed change! Very interested to see how the other internal refactoring might also work.

and unify it all in one class The only thing I had to disable is the 'custom sampler' test in test_misc

segasai · 2024-02-14T23:11:27Z

Thanks @joshspeagle
Just out of curiosity i've actually refactored nestedsamplers. It was surprisingly easy to get rid of those classes there which removed few hundred lines (of codes + comments).
I am not 100%, but 95% certain that's a good change just because it's easier to understand it now (and there was so little different between those samplers)

segasai · 2024-02-18T14:54:54Z

And if I bump the number of threads to 36 , it takes 50 seconds. I.e. basically it scales properly, while previously uniform sampler basically didn't really scale with multiple threads.

import numpy as np
import dynesty
import multiprocessing as mp

nlive = 1000


def loglike_egg(x):
    logl = ((2 + np.cos(x[0] / 2) * np.cos(x[1] / 2))**5)
    return logl


def prior_transform_egg(x):
    return x * 10 * np.pi


LOGZ_TRUTH = 235.856


def test_bounds():
    bound, sample = 'single', 'unif'
    # stress test various boundaries
    ndim = 2
    rstate = np.random.default_rng(444)
    with mp.Pool(4) as poo:
        sampler = dynesty.NestedSampler(loglike_egg,
                                        prior_transform_egg,
                                        ndim,
                                        nlive=nlive,
                                        bound=bound,
                                        sample=sample,
                                        rstate=rstate,
                                        pool=poo,
                                        queue_size=4)
        sampler.run_nested(dlogz=0.1, print_progress=True)
        assert (abs(LOGZ_TRUTH - sampler.results.logz[-1])
                < 5. * sampler.results.logzerr[-1])


if __name__ == '__main__':
    test_bounds()

rearrange sampler arguments to make them more logical

segasai · 2024-02-19T10:26:46Z

It would be great to have some review of this patch @joshspeagle if you have time, before the patch gets bigger.

My current thinking is the patch is mostly positive with a few negatives listed below:

Previously I had a test of custom sampler like that _SAMPLERS["custom"] = MultiEllipsoidSampler. I had to disable this, but I don't actually know if anyone in the wild is using this functionality.
The new logic in propose_live has too many ifs() for my liking because it has to deal with combinations of different bounds/samplers. I think this needs to be cleaned up.
kwargs is used to communicate the bound and a few other variables to sample_bound_unif. Maybe that needs to be changed and we need to expand SampleArgument

dynesty/py/dynesty/sampling.py

Line 24 in 3f10025

SamplerArgument = namedtuple('SamplerArgument', [

rather than abuse kwargs
Renaming of a few attributes (there were .method/.sampling attributes, now there is only .sampling). The .bound is renamed to .bound_list.
Some interface change to bounding classes, so they can be used interchangeably. Maybe having an BoundingInterface class is a good idea...
I don't have an actual example, but I could imagine the case where the new implementation is slower. Maybe the case of very high acceptance rate, very fast logl function, and very large bound, in this case the overhead from pickling the bound and sending it to the pool worker may be significant. Probably this is not a big worry, since if acceptance fraction is high, things are easy no matter what.

Among things I'd like to do, but I don't want to do in this patch: I think with this patch it's becoming easier to do #391. We already now can pack update_slice or update_rwalk in SliceSampler RWalkSampler objects and make sure that things like history

dynesty/py/dynesty/nestedsamplers.py

Line 145 in 3f10025

self.slice_history = {'ncontract': 0, 'nexpand': 0}

is stored inside those objects. But I'd prefer to do that separately, as this requires more thought.

(some tests may fail)

Also there was bug fix in the boundary isinstance check, and change how blob is propagated to samplers

…estedsampler

Copilot

Pull Request Overview

This PR introduces a new sampler interface for dynesty with a focus on parallel uniform sampling capabilities. The changes significantly refactor the sampling architecture by:

Replacing function-based samplers with class-based InternalSampler interface
Moving from nestedsamplers.py to sampling.py for sampler implementations
Updating the bound interface and attribute naming (bound_list vs bound)

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
py/dynesty/sampling.py	Complete rewrite introducing InternalSampler class hierarchy and new sampler interface
py/dynesty/sampler.py	Major refactor integrating new sampler interface and bound handling
py/dynesty/dynesty.py	Updated to use new sampler classes and removed deprecated functionality
py/dynesty/nestedsamplers.py	File completely removed as functionality moved to sampling.py
py/dynesty/utils.py	Removed deprecated old_stopping_function and improved error handling
tests/*	Multiple test files updated to use new sampler interface

Comments suppressed due to low confidence (2)

tests/test_sampling.py:84

The 'hslice' sampler was removed from the new interface but the test removal is incomplete. Ensure all references to deprecated samplers are properly cleaned up.

        'rslice': ds.RSliceSampler().sample,

py/dynesty/utils.py:2168

The attribute name was changed from 'M' to 'mapper', but this should be documented as it's a breaking change in the API.

        cursamp.mapper = mapper

Copilot · 2025-07-21T02:45:52Z

+class SliceSampler(InternalSampler):
+
+    def __init__(self, **kwargs):
+        super().__init__(**kwargs)
+        # Initialize slice parameters.
+        slices = kwargs.get('slices', 5)
+        self.slice_history = {'ncontract': 0, 'nexpand': 0}


The slice_history dictionary is duplicated between SliceSampler and RSliceSampler classes. Consider extracting this to a common base class or mixin to reduce code duplication.

Suggested change

class SliceSampler(InternalSampler):

def __init__(self, **kwargs):

super().__init__(**kwargs)

# Initialize slice parameters.

slices = kwargs.get('slices', 5)

self.slice_history = {'ncontract': 0, 'nexpand': 0}

class SliceSampler(SliceHistoryMixin, InternalSampler):

def __init__(self, **kwargs):

super().__init__(**kwargs)

# Initialize slice parameters.

slices = kwargs.get('slices', 5)

Copilot · 2025-07-21T02:45:52Z

+class RSliceSampler(InternalSampler):
+
+    def __init__(self, **kwargs):
+        super().__init__(**kwargs)
+        # Initialize slice parameters.
+        slices = kwargs.get('slices', 5)
+        self.slice_history = {'ncontract': 0, 'nexpand': 0}


The slice_history dictionary is duplicated between SliceSampler and RSliceSampler classes. Consider extracting this to a common base class or mixin to reduce code duplication.

Copilot · 2025-07-21T02:45:52Z

+        # the minimum number points we want with finite logl
+        # we want want at least ndim+1, because we want
+        # to be able to constraint the ellipsoid
+        # Note that if nlive <ndim+ 1 this doesn't really make sense
+        # but we should have warned the user earlier, so they are on their own
+        # And the reason we have max(ndim+1, X ) is that we'd like to get at
+        # least X points as otherwise the poisson estimate of the volume will
+        # be too large.
+        # The reason why X is min(nlive-20, 100) is that we want at least 100
+        # to have reasonable volume accuracy of ~ 10%
+        # and the reason for nlive-20 is because if nlive is 100, we don't want
+        # all points with finite logl, because this leads to issues with
+        # integrals and batch sampling in plateau edge tests
+        # The formula probably should be simplified


The complex logic for determining min_npoints needs clearer documentation. The current comment is confusing and could benefit from breaking down the formula into separate documented steps.

Suggested change

# the minimum number points we want with finite logl

# we want want at least ndim+1, because we want

# to be able to constraint the ellipsoid

# Note that if nlive <ndim+ 1 this doesn't really make sense

# but we should have warned the user earlier, so they are on their own

# And the reason we have max(ndim+1, X ) is that we'd like to get at

# least X points as otherwise the poisson estimate of the volume will

# be too large.

# The reason why X is min(nlive-20, 100) is that we want at least 100

# to have reasonable volume accuracy of ~ 10%

# and the reason for nlive-20 is because if nlive is 100, we don't want

# all points with finite logl, because this leads to issues with

# integrals and batch sampling in plateau edge tests

# The formula probably should be simplified

# Determine the minimum number of points (`min_npoints`) with finite logl:

# 1. We need at least `ndim + 1` points to constrain the ellipsoid.

# - If `nlive < ndim + 1`, this is problematic, but the user should

# have been warned earlier.

# 2. To ensure reasonable Poisson volume estimates, we aim for at least `X` points:

# - `X` is defined as `min(nlive - 20, 100)`:

# a. `100` ensures ~10% volume accuracy.

# b. `nlive - 20` avoids using all `nlive` points with finite logl,

# which can cause issues with integrals and batch sampling in

# plateau edge tests.

# 3. The final formula combines these constraints:

# - `max(ndim + 1, X)` ensures we meet both the ellipsoid constraint

# and the Poisson volume accuracy requirement.

# - `min(nlive, ...)` ensures we do not exceed the total number of live points.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

segasai · 2025-07-21T10:10:19Z

I think it is time for merging this and start to release the new versions. I am confident this is ready for use and tests are adequate.
In term of new features using the new interface, I did not have time to implement much, but I'll try to put them in while in the main branch, as i have time.
The easiest one is for the sampler to return additional information if needed. I just need to establish an interface for it.

segasai added 5 commits February 13, 2024 20:13

test sampler method for bound uniform sampler

96dcb06

update bounding infrastructure

7e175e0

nestedsamplers classes can probably be removed at some point

update t make the tests pass

09b4f5d

fix the unitcheck

8b6a4f1

now all the tests pass

e5014dd

clean up dead code

bc121aa

segasai added 4 commits February 14, 2024 22:02

put back the warning about inefficient sampling

38141cf

get rid of all the classes in nested samplers

b13c34b

and unify it all in one class The only thing I had to disable is the 'custom sampler' test in test_misc

cleaning

0d20449

unify sup/ball boundary interface

21ce546

segasai added 8 commits February 14, 2024 23:37

get rid of nestedsamplers completely

df6b33f

update docs

365df76

there were two variables .method and .sampling storing the same thing

a32ad73

propose_point is always propose_live

c378364

cleaner exception

59a7313

cleanup

8ed4261

fix typo

8f3a05f

the warning was repeated many times, fix that

23d09b8

segasai added 6 commits February 18, 2024 18:33

update comments

6f0953c

rearrange sampler arguments to make them more logical

fix doc generation issue

2f30c7c

fix doc building

eb4a8c7

update comments

bdb364a

completely rename to .method to .sampling

244c65d

fix comments

1723bcc

segasai requested a review from joshspeagle February 19, 2024 10:03

segasai added 24 commits April 25, 2025 03:18

update test

f46f74c

replace a few final M's by mapper

e36507f

protection against errors

091ca26

update bounding interfact to allow ndim only initialisation

f4b76ea

fix last commit

645f6b6

update test

bf0c738

directly access the dictionary

1b9a260

refactor for clarity

a7981dd

get rid .get() in sampling

dbfe893

typos

cb81c9b

update the bound update interface

f757828

(some tests may fail)

fix failing tests

bb69c97

try to fix test error

35d56b9

make Dynamicsampler closer to actual Sampler

96882b9

get rid of walks/slices variables

f9d797b

Unify sampler initialisation

37b31de

Also there was bug fix in the boundary isinstance check, and change how blob is propagated to samplers

fix some warnings

642bd95

get rid of save_samples. it was poorly tested/not needed

37da9a0

finish up the sampler interface and how samplers are initialised in n…

35a8965

…estedsampler

fix walks/slices options

9daf7ac

formatting

6082747

fix the bug fixed in master

9977da3

Merge branch 'master' into unif_parallel

5d61dbf

fix comments and remove some todos

7cb2b18

segasai requested a review from Copilot July 21, 2025 02:44

Copilot AI reviewed Jul 21, 2025

View reviewed changes

Update py/dynesty/sampling.py

dc336be

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

segasai merged commit 075ed81 into master Jul 21, 2025
6 checks passed

segasai deleted the unif_parallel branch October 4, 2025 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parallel uniform sampling + New sampler interface#471

Parallel uniform sampling + New sampler interface#471
segasai merged 87 commits into
masterfrom
unif_parallel

segasai commented Feb 14, 2024

Uh oh!

coveralls commented Feb 14, 2024 •

edited

Loading

Uh oh!

joshspeagle commented Feb 14, 2024

Uh oh!

segasai commented Feb 14, 2024

Uh oh!

segasai commented Feb 18, 2024 •

edited

Loading

Uh oh!

segasai commented Feb 19, 2024

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI Jul 21, 2025

Uh oh!

Copilot AI Jul 21, 2025

Uh oh!

Copilot AI Jul 21, 2025

Uh oh!

segasai commented Jul 21, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

segasai commented Feb 14, 2024

Uh oh!

coveralls commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 7952108689

Details

💛 - Coveralls

Uh oh!

joshspeagle commented Feb 14, 2024

Uh oh!

segasai commented Feb 14, 2024

Uh oh!

segasai commented Feb 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

segasai commented Feb 19, 2024

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

segasai commented Jul 21, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

coveralls commented Feb 14, 2024 •

edited

Loading

segasai commented Feb 18, 2024 •

edited

Loading