Skip to content

Conversation

@jackkohm
Copy link

The second sub-iteration in EnsembleSampler.sample incorrectly updated the second half of chains against itself instead of the first half. Add a deterministic regression test that fails under the previous split logic and verifies complementary-half updates.

The second sub-iteration in EnsembleSampler.sample incorrectly updated the second half of chains against itself instead of the first half. Add a deterministic regression test that fails under the previous split logic and verifies complementary-half updates.
@martinjankowiak
Copy link
Collaborator

cc @amifalk

@amifalk
Copy link
Contributor

amifalk commented Feb 10, 2026

Nice catch! My tiny nit is that it was a bit hard to initially parse that the first two chains have values that aren't used in the test case. Initializing z to [0.0], [0.0], [10.0], [11.0]] or [nan], [nan], [10.0], [11.0]]might help signpost the logic of the test, i.e that you start by copying [10, 11] + 1 into the first two chains, and then copy [11, 12] + 1 into the last two chains.

@jackkohm
Copy link
Author

@amifalk implemented, thanks for the suggestion.
I updated the test fixture to make the first-half values explicit and irrelevant ([[0.0], [0.0], [10.0], [11.0]]) and added a clarifying comment above it so the update sequence is easier to parse.
Also re-ran the focused regression test (test_ensemble_sampler_uses_complementary_halves) and it passes.

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