Conversation
Each batch derived a root of its own, so a study could not be repeated and the same max_simulations reached different samples per batch size. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1198 +/- ##
========================================
Coverage 91.58% 91.58%
========================================
Files 132 132
Lines 18404 18404
========================================
+ Hits 16855 16856 +1
+ Misses 1549 1548 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
simulate_convergencehad no way to take a seed, so each batch derived a root of its own and the study could not be repeated. Addresses #1077.Pull request type
Checklist
ruff check/ruff format --check,pylint) has passed locallyCHANGELOG.md: one line, since the workflow that would have written it has not run since CI: run the changelog job for pull requests from forks #1112 (BUG: the changelog workflow stopped running, and CHANGELOG.md is 37 merged pull requests behind #1173).Current behavior
simulate_convergenceruns batches until the confidence interval settles, callingsimulate(append=True)for each one. It takes no seed and passes none on, so every batch draws fresh entropy. Two consequences: the same study cannot be run twice, and the samemax_simulationsreaches a different set of samples depending on thebatch_sizethat divided it up, which is the opposite of what a batch size should mean.New behavior
It takes a keyword-only
random_seedand hands it to each batch. The seed fixes the study rather than the batch: every batch continues the root the rows already record, so a givenmax_simulationsreaches the same samples whicheverbatch_sizewas used to get there. Leaving it out keeps today's behavior, which draws fresh entropy and reproduces nothing.The eight lines that do this are a pass-through. What makes them mean something is #1187, which merged last week: an append now continues the root its rows were drawn with rather than starting a second one, and a simulation takes its seed from its own index rather than from the worker or the batch that happened to run it. Before that, passing the same seed to every batch would have restarted the sequence each time.
Breaking change
random_seeddefaults toNone, which is what every existing call already does.Additional information
Three tests. One runs the same study twice and compares what was drawn; one runs the same
max_simulationsunder two different batch sizes and compares those; the third is the control, that a study with no seed still runs. Accepting the seed and not passing it on turns the first two red and leaves the control green.This began as a branch from August that carried thirteen commits. Eleven of them have since merged, as #1181 and #1187, so only the two here are new. I took those onto current
developrather than rebasing the branch, since the diff againstdevelopwas dominated by what the old base was missing rather than by anything this adds.Verified on
a50d07d5:The ten are the same ten on both, all of them optional dependencies this machine does not have.