BUG: give each CustomSampler its own stream instead of the model's seed - #1102
BUG: give each CustomSampler its own stream instead of the model's seed#1102thc1006 wants to merge 7 commits into
Conversation
Every sampler on a model was reset with the model's seed, so two backed by default_rng started from identical state and drew identical underlying values. Not nearly identical, the same to every digit: two Gaussians with different means and spreads both produced the deviate 0.466220770577340. A study varying two parameters that way is varying one, and the correlation it reports between them is an artefact of the seeding. Each sampler now gets a child derived from the model's seed and the input's name. Keyed by name rather than position so declaring another parameter does not move the streams of the ones already there, and crc32 rather than hash because hash is not stable across processes. The documented wind X/Y wrappers are unaffected. Their correlation comes from sharing one samples_list, not from sharing a seed, so handing them separate children leaves it intact: measured 0.7010 against the covariance's 0.6981. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1102 +/- ##
===========================================
+ Coverage 82.18% 82.59% +0.41%
===========================================
Files 122 128 +6
Lines 16355 16583 +228
===========================================
+ Hits 13441 13697 +256
+ Misses 2914 2886 -28 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Two problems with the first version of this, both found in review. CRC32 is 32 bits, and a collision puts two samplers back on one stream, which is the bug the keying exists to prevent. `wd4s4xka50` and `p56cjcee10` are both valid identifiers with CRC32 1560575156, and both derived the same seed. The name is length-prefixed into spawn-key words now, which no two names share, and the child is kept at its full 128 bits to match the Monte Carlo seeding rather than being cut to 64. Samplers can also share one generator on purpose, as the documented wind pair does, and each reset overwrites the last. With one seed per name, whichever was reset last decided the stream, so the same seed meant different runs depending on the order the model was declared in. Seeding is its own pass over sorted names now. The pass is separate from the validation loop deliberately. That loop's order sets __dict__, and so the order every other input is drawn in, so sorting it would have moved the samples of every model with a tuple in it. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
|
Both of these were right and both are fixed in CRC32 collides. 32 bits is not enough for a stream domain, and a collision puts two samplers back on one stream, which is exactly what this PR exists to prevent. The reproducer holds: My "107 built-in names, no collisions" was the wrong test. It says nothing about a subclass, a future field, or a name a user picks. The name is length-prefixed into spawn-key words now, which no two names can share, and the child keeps its full 128 bits instead of being cut to 64. That also lines it up with the width #1054 uses rather than having the two disagree. The shared generator was order-coupled, and that one I introduced. Before this branch both wrappers got the same seed, so order did not matter. One seed per name made whichever wrapper reset last decide the stream: Seeding is its own pass over sorted names now, so the shared generator lands on the same seed whichever way the model was written. Where I did not follow the suggestionYou proposed a It fixes what I broke. What it leaves is that two wrappers reading one generator take successive values, so swapping the declaration swaps which wrapper gets which number. That is inherent to sharing a generator rather than a seeding property, it is true on One thing worth flaggingMy first attempt at the order fix was to sort the main validation loop, which looked like a one-word change. It is not: that loop's order sets Local: ruff clean, |
Two wrappers can share one generator on purpose, as the documented wind pair do. One seed per name reset that generator once per wrapper, so every seed but the last was discarded and the group's stream was decided by whichever member sorted last. Adding a third wrapper to the same generator therefore moved the first two, which name keying is meant to prevent. CustomSampler gains a `seed_group` property, `self` by default, so a wrapper can say which generator it shares. Members of a group are seeded once between them, with the seed derived from all their names rather than from whichever went last. The documented wind wrappers declare it. Before, resetting six times for three wrappers and moving the pair when a third arrived. After, once, and adding an independent sampler leaves the group where it was. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
|
You were right and I was wrong to argue it away. My case for the sorted pass was that it fixed what I had broken and the rest was inherent. It did fix the order coupling, but it left something I had also introduced, and I did not go looking for it: Before this branch every sampler took the model's seed, so adding a member to a shared group changed nothing for the others. Keying by name protects independent samplers from exactly that, and left the shared group unprotected, because its stream was whatever the last-sorted member's seed made it. So the inconsistency was mine, not inherent.
Adding a member to a group still moves that group's stream. That one does look inherent to me: the group is keyed by its members, and a different set of members is a different group. Say if you disagree. One thing I did and then undidGrouping keys on The test passed with the fix removed. In this loop's actual shape the temporaries do not collide, so the test proved nothing and I have taken it out. The reference is still held, because keying on an Local: ruff clean, |
Only RuntimeError was caught, and the seed handed over is now 128 bits, which
the legacy numpy.random.RandomState refuses:
ValueError: Seed must be between 0 and 2**32 - 1
Before this branch a sampler received the model's seed, usually a small int,
so RandomState took it. A sampler built on RandomState therefore breaks here,
and used to break with a bare ValueError that named nothing.
The seed stays 128 bits, since that is what keeps the streams apart and what
default_rng, the documented choice, takes. The error now says which input the
sampler belongs to and keeps the original as its cause.
Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
…discards Three things from review, all small. The documented bivariate generator filled a 1000-pair cache inside reset_seed. With per-index seeding that reset happens once per simulation, so a study built on this example generated a thousand pairs and used one, every time. 0.099 ms each, about 10 s over 100k simulations. `top_up` already fills the shortfall on first use, so the eager fill is gone and the cache starts empty. The group reset went through the first member rather than the group, which assumes every member resets identically and keeps nothing of its own. The group holds the shared state, so it is reset directly when it knows how, and the member is the fallback. `_sampler_seed` now sorts the names itself. The caller does today, and a future one that forgets would hand a single group two different seeds. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
|
Three more in The documented example filled a cache it threw away. The cache starts empty now and The group reset went through a member. It dispatched to the first sampler in the group rather than to the group, which assumes every member resets identically and holds nothing of its own. The group owns the shared generator, so it is reset directly when it knows how, with the member as the fallback.
I have also removed an earlier comment of mine here that said the conflict with #1054 was one line of imports. That was true of Local: ruff clean, |
Two rules the property invites breaking, both silent. Identity has to be stable. Building the answer on each call, which returning from a property makes easy, gives every member a different identity and puts each back in a group of its own: a two-member group goes from one reset to two. A group belongs to one model. Declaring the same generator on two models has them both seed it, and the later one wins, which is the overwrite the grouping exists to prevent. The documented wind pair already returns a stored attribute, so the example teaches the stable form. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
|
Pushed Two ways to break it, both silent, both measured: Identity has to be stable across calls. Returning from a property makes it easy to build the answer each time, and then every member has a different identity and goes back into a group of its own. A two-member group drops from one reset to two: A group belongs to one model. Declaring the same generator on two models has them both seed it and the later one wins, which is the overwrite the grouping is here to prevent. I did not add tests for either. The first is a consequence of I also considered detecting an unstable group and warning, and decided against it. It degrades to per-member seeding, which is the default anyway, so nothing goes wrong that would not have gone wrong without the property. The wind pair in the docs returns a stored attribute, so the example already teaches the stable form. Local: ruff clean, pylint 10.00/10 exit 0, stochastic unit suite 39 passed. |
The automation that normally writes it cannot run on a pull request from a fork, which is RocketPy-Team#1101, so this one is by hand. It is a breaking change and the entry says so: fixed-seed CustomSampler baselines move, and a sampler built on the legacy RandomState has to move to default_rng because the seed it now receives is 128 bits wide. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
|
Added the changelog entry directly here, as you suggested, since the automation cannot run on a fork pull request until #1101 is fixed. Head is It records both migrations: fixed-seed On the merge order, that matches what I found independently: #1104 first, then this branch updated so the Sphinx warnings-as-errors job actually runs against the On |
1 participant
Addresses #1093. Not
Closes, because the keyword only fires when a pull request targets the default branch and this targetsdevelop.Pull request type
Checklist
ruff check/ruff format --check/pylint rocketpy/ tests/ docs/) has passed locallypylintexits 0,pytest tests/unit tests/integrationis 2049 passed, 44 skipped.Current behavior
_validate_custom_samplerended insampler.reset_seed(seed), andseedwas the model's own, handed unchanged to every sampler on it. A sampler written the way the documentation teaches buildsnp.random.default_rng(seed), so two of them started from identical state and drew identical values. Not nearly identical, the same to every digit:Those are the standard normal deviates behind each draw; the last digit is the scaling. A study varying both parameters was varying one, and any correlation it reported between them was an artefact of the seeding.
New behavior
Each sampler, or each group of samplers that share a generator, gets a child derived from the model's seed and the input names:
Keyed by name rather than position, so declaring another parameter does not move the streams already there. The name is length-prefixed into spawn-key words, which no two names share, rather than hashed: a collision would put two samplers back on one stream, which is the bug this exists to prevent.
wd4s4xka50andp56cjcee10are both valid identifiers whose CRC32 is 1560575156, and an earlier version of this gave them the same seed.The child keeps its full 128 bits, matching the width the Monte Carlo seeding uses rather than being cut to 64.
Samplers that share a generator
docs/user/custom_sampler.rstdocuments two wrappers over one bivariate generator, so that wind X and wind Y stay correlated.CustomSamplergains aseed_groupproperty,selfby default, and those wrappers return the generator they share. A group is seeded once between its members, from a seed derived from all their names.Resetting each member in turn discarded every seed but the last and left the group's stream decided by whichever member went last, so adding a third wrapper moved the first two. The group is reset directly when it knows how, rather than through one member, since the member cannot be assumed to reset identically or to hold nothing of its own.
The correlation is untouched by any of this. It comes from sharing one
samples_list, not from sharing a seed:That is the documented pattern executed out of the
.rstrather than retyped.The example also filled a 1000-pair cache inside
reset_seed. With per-index seeding that reset happens once per simulation, so it built a thousand pairs to use one, every time. The cache starts empty now andtop_upfills the shortfall on first use.Tests
Sixteen in
tests/unit/stochastic/test_custom_sampler.py. Each fix has a mutation that fails a named test and takes nothing else:two_samplers_do_not_draw_the_same_deviatetwo_names_that_a_hash_would_collidethe_sampler_seed_keeps_the_full_widtha_shared_generator_lands_on_the_same_seeda_shared_group_is_seeded_oncea_group_that_can_reset_itself_is_reset_directlya_group_key_does_not_depend_on_the_orderRuntimeErroronlyRandomStatetestThe fresh-entropy one is the one worth having. Independence is easy to get by throwing the seed away, and that passes the first test while losing the property the class exists for.
Breaking change
Sampled values change for any study using a
CustomSampler, since each now receives a different seed. Anything leaning on the accidental correlation will see the parameters move apart, which is the fix rather than a regression, but a baselined study will notice.A sampler built on the legacy
numpy.random.RandomStatewill now fail: it refuses seeds above 2**32-1 and these are 128 bits. The error names the input it belongs to and keeps the original as its cause, rather than surfacing as a bareValueError.Additional information
Found while reviewing #1054. The two overlap in
rocketpy/stochastic/stochastic_model.py: that PR changes the base model's list sampling and nominal snapshot, this one changes the sampler lifecycle. They merge cleanly today and I ran the merged tree rather than assuming, 291 passed and 4 skipped, but whichever lands second wants a rebase rather than a trust in that.Related: #1096, whose eager cache this takes a bite out of.