From 6d66bde5d9765684a695a3c7b9cb91e239bd2920 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 8 Aug 2026 10:54:14 +0800 Subject: [PATCH 1/8] BUG: give each CustomSampler its own stream instead of the model's seed 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> --- rocketpy/stochastic/stochastic_model.py | 17 +++- tests/unit/stochastic/test_custom_sampler.py | 83 ++++++++++++++++++++ 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/rocketpy/stochastic/stochastic_model.py b/rocketpy/stochastic/stochastic_model.py index ca26f6578..c2eb28f49 100644 --- a/rocketpy/stochastic/stochastic_model.py +++ b/rocketpy/stochastic/stochastic_model.py @@ -4,6 +4,7 @@ """ from random import choice +from zlib import crc32 import numpy as np @@ -12,6 +13,20 @@ from ..tools import get_distribution + +def _sampler_seed(seed, input_name): + """Derive one sampler's seed from the model's, so it gets its own stream. + + Keyed by the input's name rather than its position, so declaring another + parameter does not move the stream of the ones already there. ``crc32`` + because it is stable across processes, which ``hash`` is not. + """ + root = np.random.SeedSequence( + entropy=seed, spawn_key=(crc32(input_name.encode("utf-8")),) + ) + return int(root.generate_state(1, dtype=np.uint64)[0]) + + # TODO: Stop using assert in production code. Use exceptions instead. # TODO: Each validation method should have a test case. @@ -467,7 +482,7 @@ def _validate_custom_sampler(self, input_name, sampler, seed=None): If the input is not in a valid format. """ try: - sampler.reset_seed(seed) + sampler.reset_seed(_sampler_seed(seed, input_name)) except RuntimeError as e: raise RuntimeError( f"An error occurred in the 'reset_seed' method of {input_name} CustomSampler" diff --git a/tests/unit/stochastic/test_custom_sampler.py b/tests/unit/stochastic/test_custom_sampler.py index 90774ac50..2994b0e24 100644 --- a/tests/unit/stochastic/test_custom_sampler.py +++ b/tests/unit/stochastic/test_custom_sampler.py @@ -1,3 +1,8 @@ +import numpy as np +import pytest + +from rocketpy.stochastic import StochasticRocket +from rocketpy.stochastic.custom_sampler import CustomSampler from rocketpy.environment.environment import Environment @@ -19,3 +24,81 @@ class creates a StochasticEnvironment object from the randomly generated """ obj = stochastic_environment_custom_sampler.create_object() assert isinstance(obj, Environment) + + +class _Gaussian(CustomSampler): + """A sampler of the shape the documentation teaches.""" + + def __init__(self, mean, sd): + self.mean, self.sd = mean, sd + self.rng = np.random.default_rng() + + def sample(self, n_samples=1): + return list(self.rng.normal(self.mean, self.sd, n_samples)) + + def reset_seed(self, seed=None): + self.rng = np.random.default_rng(seed) + + +def _deviates(drawn): + """The standard normal behind each draw, so samplers with different means + and spreads can still be compared.""" + return ( + (drawn["mass"] - 14.426) / 0.5, + (drawn["radius"] - 0.0635) / 0.001, + ) + + +def _two_sampler_model(calisto_robust): + return StochasticRocket( + rocket=calisto_robust, + mass=_Gaussian(14.426, 0.5), + radius=_Gaussian(0.0635, 0.001), + ) + + +def test_two_samplers_do_not_draw_the_same_deviate(calisto_robust): + """Every sampler on a model used to be reset with the model's own seed, so + two backed by ``default_rng`` started from the same state and drew the same + underlying value. Not nearly identical: the same, to every digit.""" + model = _two_sampler_model(calisto_robust) + model._set_stochastic(4242) + + mass, radius = _deviates(next(model.dict_generator())) + + assert mass != pytest.approx(radius, abs=1e-12) + + +def test_a_seed_still_reproduces_the_same_samples(calisto_robust): + """The control. Independence must not have been bought with fresh entropy + per reseed, which would decorrelate the samplers and lose the seed.""" + model = _two_sampler_model(calisto_robust) + + model._set_stochastic(4242) + first = _deviates(next(model.dict_generator())) + model._set_stochastic(4242) + again = _deviates(next(model.dict_generator())) + model._set_stochastic(99) + other = _deviates(next(model.dict_generator())) + + assert first == again + assert first != other + + +def test_adding_a_parameter_leaves_the_others_where_they_were(calisto_robust): + """Seeds are keyed by the input's name, not its position, so declaring one + more sampler does not move the streams of the ones already there.""" + model = _two_sampler_model(calisto_robust) + model._set_stochastic(4242) + before = _deviates(next(model.dict_generator())) + + wider = StochasticRocket( + rocket=calisto_robust, + mass=_Gaussian(14.426, 0.5), + radius=_Gaussian(0.0635, 0.001), + inertia_11=_Gaussian(6.321, 0.1), + ) + wider._set_stochastic(4242) + after = _deviates(next(wider.dict_generator())) + + assert after == before From 6211dbb656d9914b6ebbf38070cc4826d7dcbcb2 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 8 Aug 2026 12:30:38 +0800 Subject: [PATCH 2/8] BUG: make the sampler key collision-free and the shared case order-free 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> --- rocketpy/stochastic/stochastic_model.py | 76 ++++++++++----- tests/unit/stochastic/test_custom_sampler.py | 97 +++++++++++++++++++- 2 files changed, 151 insertions(+), 22 deletions(-) diff --git a/rocketpy/stochastic/stochastic_model.py b/rocketpy/stochastic/stochastic_model.py index c2eb28f49..ae582896b 100644 --- a/rocketpy/stochastic/stochastic_model.py +++ b/rocketpy/stochastic/stochastic_model.py @@ -4,7 +4,6 @@ """ from random import choice -from zlib import crc32 import numpy as np @@ -14,17 +13,33 @@ from ..tools import get_distribution +def _name_as_spawn_key(input_name): + """Encode a name into spawn-key words with no two names sharing an encoding. + + A hash would be shorter, but a collision puts two samplers back on one + stream, which is the bug this keying exists to prevent. The length prefix + is what makes it injective. + """ + encoded = input_name.encode("utf-8") + payload = len(encoded).to_bytes(4, "little") + encoded + payload += b"\0" * (-len(payload) % 4) + return tuple( + int.from_bytes(payload[at : at + 4], "little") + for at in range(0, len(payload), 4) + ) + + def _sampler_seed(seed, input_name): """Derive one sampler's seed from the model's, so it gets its own stream. Keyed by the input's name rather than its position, so declaring another - parameter does not move the stream of the ones already there. ``crc32`` - because it is stable across processes, which ``hash`` is not. + parameter does not move the stream of the ones already there. """ root = np.random.SeedSequence( - entropy=seed, spawn_key=(crc32(input_name.encode("utf-8")),) + entropy=seed, spawn_key=_name_as_spawn_key(input_name) ) - return int(root.generate_state(1, dtype=np.uint64)[0]) + words = root.generate_state(4, dtype=np.uint32) + return sum(int(word) << (32 * position) for position, word in enumerate(words)) # TODO: Stop using assert in production code. Use exceptions instead. @@ -97,6 +112,8 @@ def _set_stochastic(self, seed=None): self.__random_number_generator = np.random.default_rng(seed) self.last_rnd_dict = {} + self._reset_custom_samplers(seed) + # TODO: This code block is too complex. Refactor it. # TODO: Resetting a instance should not require re-validation. for input_name, input_value in self.__stochastic_dict.items(): @@ -104,9 +121,7 @@ def _set_stochastic(self, seed=None): attr_value = None if input_value is not None: if "factor" in input_name: - attr_value = self._validate_factors( - input_name, input_value, seed - ) + attr_value = self._validate_factors(input_name, input_value) elif input_name not in self.exception_list: if isinstance(input_value, tuple): attr_value = self._validate_tuple(input_name, input_value) @@ -116,7 +131,7 @@ def _set_stochastic(self, seed=None): attr_value = self._validate_scalar(input_name, input_value) elif isinstance(input_value, CustomSampler): attr_value = self._validate_custom_sampler( - input_name, input_value, seed + input_name, input_value ) else: raise AssertionError( @@ -303,7 +318,7 @@ def _validate_scalar(self, input_name, input_value, getattr=getattr): # pylint: get_distribution("normal", self.__random_number_generator), ) - def _validate_factors(self, input_name, input_value, seed): + def _validate_factors(self, input_name, input_value): """ Validate factor arguments. @@ -332,7 +347,7 @@ def _validate_factors(self, input_name, input_value, seed): elif isinstance(input_value, list): return self._validate_list_factor(input_name, input_value) elif isinstance(input_value, CustomSampler): - return self._validate_custom_sampler(input_name, input_value, seed) + return self._validate_custom_sampler(input_name, input_value) else: raise AssertionError( f"`{input_name}`: must be either a tuple or listor a custom sampler" @@ -463,31 +478,50 @@ def _validate_positive_int_list(self, input_name, input_value): isinstance(member, int) and member >= 0 for member in input_value ), f"`{input_name}` must be a list of positive integers" - def _validate_custom_sampler(self, input_name, sampler, seed=None): + def _reset_custom_samplers(self, seed): + """Give every sampler its own stream, in an order that does not move. + + Sorted rather than declaration order, because two samplers can share + one generator on purpose, as the documented wind pair does, and then + whichever is reset last decides the stream. Its own pass rather than + the loop below, whose order sets ``__dict__`` and so the order every + other input is drawn in. + """ + for input_name in sorted(self.__stochastic_dict): + sampler = self.__stochastic_dict[input_name] + if not isinstance(sampler, CustomSampler): + continue + try: + sampler.reset_seed(_sampler_seed(seed, input_name)) + except RuntimeError as error: + raise RuntimeError( + f"An error occurred in the 'reset_seed' method of " + f"{input_name} CustomSampler" + ) from error + + def _validate_custom_sampler(self, input_name, sampler): """ Validate a custom sampler. + Seeding is not done here. It happens in ``_reset_custom_samplers``, + which runs in a fixed order because two samplers can share one + generator and whichever is reset last decides the stream. + Parameters ---------- input_name : str Name of the input argument. sampler : CustomSampler object Custom sampler provided by the user - seed : int, optional - Seed for the random number generator. The default is None Raises ------ AssertionError If the input is not in a valid format. """ - try: - sampler.reset_seed(_sampler_seed(seed, input_name)) - except RuntimeError as e: - raise RuntimeError( - f"An error occurred in the 'reset_seed' method of {input_name} CustomSampler" - ) from e - + assert isinstance(sampler, CustomSampler), ( + f"`{input_name}` must be a CustomSampler, not {type(sampler).__name__}" + ) return sampler def _validate_airfoil(self, airfoil): diff --git a/tests/unit/stochastic/test_custom_sampler.py b/tests/unit/stochastic/test_custom_sampler.py index 2994b0e24..9388cf93a 100644 --- a/tests/unit/stochastic/test_custom_sampler.py +++ b/tests/unit/stochastic/test_custom_sampler.py @@ -1,9 +1,12 @@ +from types import SimpleNamespace + import numpy as np import pytest +from rocketpy.environment.environment import Environment from rocketpy.stochastic import StochasticRocket from rocketpy.stochastic.custom_sampler import CustomSampler -from rocketpy.environment.environment import Environment +from rocketpy.stochastic.stochastic_model import StochasticModel, _sampler_seed def test_create_object(stochastic_environment_custom_sampler): @@ -102,3 +105,95 @@ def test_adding_a_parameter_leaves_the_others_where_they_were(calisto_robust): after = _deviates(next(wider.dict_generator())) assert after == before + + +class _SharedPair: + """Two wrappers over one generator, as the wind example in the docs does.""" + + def __init__(self): + self.rng = np.random.default_rng() + self.last_seed = None + self.reset_count = 0 + + def reset(self, seed): + self.rng = np.random.default_rng(seed) + self.last_seed = seed + self.reset_count += 1 + + def draw(self): + return float(self.rng.normal()) + + +class _SharedWrapper(CustomSampler): + def __init__(self, shared): + self.shared = shared + + def sample(self, n_samples=1): + return [self.shared.draw() for _ in range(n_samples)] + + def reset_seed(self, seed=None): + self.shared.reset(seed) + + +def _seed_the_shared_generator_received(declare_second_first): + shared = _SharedPair() + first, second = _SharedWrapper(shared), _SharedWrapper(shared) + inputs = ( + {"wind_y": second, "wind_x": first} + if declare_second_first + else {"wind_x": first, "wind_y": second} + ) + model = StochasticModel(SimpleNamespace(wind_x=0.0, wind_y=0.0), **inputs) + shared.reset_count = 0 # the constructor has already seeded once + model._set_stochastic(4242) + return shared.last_seed, shared.reset_count + + +def test_a_shared_generator_lands_on_the_same_seed_whatever_the_order(): + """Samplers may share one generator on purpose, and each reset overwrites + the last, so whichever is reset last decides the stream. Seeding runs in + sorted order for that reason: the same seed has to mean the same stream + whichever order the model was written in. + + On the values drawn, not the stream: two wrappers reading one generator + take successive values, so swapping the declaration swaps which wrapper + gets which. That is inherent to sharing a generator and is not seeding. + """ + ordered, reversed_ = ( + _seed_the_shared_generator_received(False), + _seed_the_shared_generator_received(True), + ) + + assert ordered == reversed_ + assert ordered[1] == 2, "each wrapper still resets the generator it wraps" + + +def test_two_names_that_a_hash_would_collide_get_different_streams(): + """Keying by a 32-bit hash put these two back on one stream, which is the + bug this keying exists to prevent. Both are valid identifiers and their + CRC32 is 1560575156.""" + assert _sampler_seed(4242, "wd4s4xka50") != _sampler_seed(4242, "p56cjcee10") + + +def test_the_sampler_seed_keeps_the_full_width(): + """128 bits, matching the width the Monte Carlo seeding uses, so a study + spawning many streams does not run into birthday collisions.""" + assert _sampler_seed(4242, "mass").bit_length() > 64 + + +def test_declaring_a_sampler_does_not_reorder_the_other_inputs(calisto_robust): + """Seeding is sorted; the validation loop below it is not. Sorting that one + too would set __dict__ alphabetically and move every tuple's draw.""" + plain = StochasticRocket(rocket=calisto_robust, mass=(14.426, 0.5)) + plain._set_stochastic(42) + before = next(plain.dict_generator()) + + with_sampler = StochasticRocket( + rocket=calisto_robust, mass=(14.426, 0.5), radius=_Gaussian(0.0635, 0.001) + ) + with_sampler._set_stochastic(42) + after = next(with_sampler.dict_generator()) + + # `radius` is declared either way, so it is in both. Only its kind changed. + assert list(after) == list(before) + assert after["mass"] == before["mass"] From 412c313ee47901e7cf5aeab214128c5a2c0b4d7e Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 8 Aug 2026 13:15:16 +0800 Subject: [PATCH 3/8] BUG: seed a shared generator once between its samplers, not once each 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> --- docs/user/custom_sampler.rst | 20 +++++++ rocketpy/stochastic/custom_sampler.py | 18 +++++++ rocketpy/stochastic/stochastic_model.py | 57 +++++++++++++------- tests/unit/stochastic/test_custom_sampler.py | 42 +++++++++++++++ 4 files changed, 117 insertions(+), 20 deletions(-) diff --git a/docs/user/custom_sampler.rst b/docs/user/custom_sampler.rst index 640167320..615ed7ea2 100644 --- a/docs/user/custom_sampler.rst +++ b/docs/user/custom_sampler.rst @@ -300,6 +300,16 @@ sample list. def __init__(self, bivariate_gaussian_generator): self.generator = bivariate_gaussian_generator + @property + def seed_group(self): + """The generator this shares with the other wrapper. + + Both return the same object, so the pair is seeded once between + them. Without this each would be seeded separately and one would + silently overwrite the other. + """ + return self.generator + def sample(self, n_samples=1): samples_list = self.generator.get_samples(n_samples, "x") return samples_list @@ -313,6 +323,16 @@ sample list. def __init__(self, bivariate_gaussian_generator): self.generator = bivariate_gaussian_generator + @property + def seed_group(self): + """The generator this shares with the other wrapper. + + Both return the same object, so the pair is seeded once between + them. Without this each would be seeded separately and one would + silently overwrite the other. + """ + return self.generator + def sample(self, n_samples=1): samples_list = self.generator.get_samples(n_samples, "y") return samples_list diff --git a/rocketpy/stochastic/custom_sampler.py b/rocketpy/stochastic/custom_sampler.py index 82a06dd9f..5b9b8598c 100644 --- a/rocketpy/stochastic/custom_sampler.py +++ b/rocketpy/stochastic/custom_sampler.py @@ -8,6 +8,24 @@ class CustomSampler(ABC): """Abstract subclass for user defined samplers""" + @property + def seed_group(self): + """The generator state this sampler shares, if it shares one. + + Samplers are independent by default and each is seeded on its own. Two + wrappers over one generator, as the correlated wind pair in the + documentation are, should both return that generator here, so the pair + is seeded once as a unit rather than one of them silently overwriting + the other's seed. + + Returns + ------- + object + Identity is what counts, not equality. ``self`` by default, which + makes every sampler its own group. + """ + return self + @abstractmethod def sample(self, n_samples=1): """Generates samples from the custom distribution diff --git a/rocketpy/stochastic/stochastic_model.py b/rocketpy/stochastic/stochastic_model.py index ae582896b..1fa26c9cf 100644 --- a/rocketpy/stochastic/stochastic_model.py +++ b/rocketpy/stochastic/stochastic_model.py @@ -13,15 +13,17 @@ from ..tools import get_distribution -def _name_as_spawn_key(input_name): - """Encode a name into spawn-key words with no two names sharing an encoding. +def _names_as_spawn_key(input_names): + """Encode names into spawn-key words that no other set of names produces. A hash would be shorter, but a collision puts two samplers back on one stream, which is the bug this keying exists to prevent. The length prefix - is what makes it injective. + before each name is what makes it injective. """ - encoded = input_name.encode("utf-8") - payload = len(encoded).to_bytes(4, "little") + encoded + payload = b"" + for name in input_names: + encoded = name.encode("utf-8") + payload += len(encoded).to_bytes(4, "little") + encoded payload += b"\0" * (-len(payload) % 4) return tuple( int.from_bytes(payload[at : at + 4], "little") @@ -29,14 +31,18 @@ def _name_as_spawn_key(input_name): ) -def _sampler_seed(seed, input_name): - """Derive one sampler's seed from the model's, so it gets its own stream. +def _sampler_seed(seed, input_names): + """Derive a seed for one sampler, or for one group that shares a generator. - Keyed by the input's name rather than its position, so declaring another - parameter does not move the stream of the ones already there. + Keyed by the names rather than by position, so declaring another parameter + does not move the stream of the ones already there. A group is keyed by all + of its members, so its stream does not depend on which of them happens to + be reset last. """ + if isinstance(input_names, str): + input_names = (input_names,) root = np.random.SeedSequence( - entropy=seed, spawn_key=_name_as_spawn_key(input_name) + entropy=seed, spawn_key=_names_as_spawn_key(tuple(input_names)) ) words = root.generate_state(4, dtype=np.uint32) return sum(int(word) << (32 * position) for position, word in enumerate(words)) @@ -479,24 +485,35 @@ def _validate_positive_int_list(self, input_name, input_value): ), f"`{input_name}` must be a list of positive integers" def _reset_custom_samplers(self, seed): - """Give every sampler its own stream, in an order that does not move. + """Give each sampler its own stream, and each shared group one between + them. - Sorted rather than declaration order, because two samplers can share - one generator on purpose, as the documented wind pair does, and then - whichever is reset last decides the stream. Its own pass rather than - the loop below, whose order sets ``__dict__`` and so the order every - other input is drawn in. + Samplers that share a generator, as the documented wind pair do, are + seeded once as a unit. Resetting each member in turn would leave every + seed but the last discarded and the group's stream decided by whichever + member happened to go last. + + Its own pass rather than the validation loop below, whose order sets + ``__dict__`` and so the order every other input is drawn in. """ + groups = {} for input_name in sorted(self.__stochastic_dict): sampler = self.__stochastic_dict[input_name] - if not isinstance(sampler, CustomSampler): - continue + if isinstance(sampler, CustomSampler): + # Held in the value as well as keyed on, because `id` is + # unique only among live objects. Defensive: a `seed_group` + # that builds its answer did not merge in practice here. + group = sampler.seed_group + shared = groups.setdefault(id(group), ([], sampler, group)) + shared[0].append(input_name) + + for names, sampler, _group in groups.values(): try: - sampler.reset_seed(_sampler_seed(seed, input_name)) + sampler.reset_seed(_sampler_seed(seed, names)) except RuntimeError as error: raise RuntimeError( f"An error occurred in the 'reset_seed' method of " - f"{input_name} CustomSampler" + f"{names[0]} CustomSampler" ) from error def _validate_custom_sampler(self, input_name, sampler): diff --git a/tests/unit/stochastic/test_custom_sampler.py b/tests/unit/stochastic/test_custom_sampler.py index 9388cf93a..7d683e118 100644 --- a/tests/unit/stochastic/test_custom_sampler.py +++ b/tests/unit/stochastic/test_custom_sampler.py @@ -197,3 +197,45 @@ def test_declaring_a_sampler_does_not_reorder_the_other_inputs(calisto_robust): # `radius` is declared either way, so it is in both. Only its kind changed. assert list(after) == list(before) assert after["mass"] == before["mass"] + + +class _GroupedWrapper(_SharedWrapper): + """A wrapper that says which generator it shares, as the docs now do.""" + + @property + def seed_group(self): + return self.shared + + +def _grouped_model(extra_independent=False): + shared = _SharedPair() + inputs = {"wind_x": _GroupedWrapper(shared), "wind_y": _GroupedWrapper(shared)} + if extra_independent: + inputs["mass"] = _Gaussian(14.426, 0.5) + obj = SimpleNamespace(**{name: 0.0 for name in inputs}) + model = StochasticModel(obj, **inputs) + shared.reset_count = 0 # the constructor has already seeded once + model._set_stochastic(4242) + return next(model.dict_generator()), shared + + +def test_a_shared_group_is_seeded_once_between_its_members(): + """Resetting each member in turn threw away every seed but the last, and + left the group's stream decided by whichever member went last. It is one + generator, so it gets one seed.""" + _, shared = _grouped_model() + + assert shared.reset_count == 1 + + +def test_an_independent_sampler_does_not_move_a_shared_group(): + """Keying by name protects independent samplers from each other. The group + has to be protected the same way, and keying it by the member that sorts + last would not have been.""" + alone, _ = _grouped_model() + alongside, _ = _grouped_model(extra_independent=True) + + assert (alone["wind_x"], alone["wind_y"]) == ( + alongside["wind_x"], + alongside["wind_y"], + ) From 06949e74127ba527a3a77215ab8cfbd6516be15e Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 8 Aug 2026 13:51:02 +0800 Subject: [PATCH 4/8] BUG: name the sampler when its generator refuses the seed 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> --- rocketpy/stochastic/stochastic_model.py | 9 ++-- tests/unit/stochastic/test_custom_sampler.py | 50 ++++++++++++++++++++ 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/rocketpy/stochastic/stochastic_model.py b/rocketpy/stochastic/stochastic_model.py index 1fa26c9cf..fd4ee84f4 100644 --- a/rocketpy/stochastic/stochastic_model.py +++ b/rocketpy/stochastic/stochastic_model.py @@ -510,10 +510,13 @@ def _reset_custom_samplers(self, seed): for names, sampler, _group in groups.values(): try: sampler.reset_seed(_sampler_seed(seed, names)) - except RuntimeError as error: + except Exception as error: + # Not just RuntimeError. The seed handed over is now 128 bits, + # which the legacy RandomState refuses with a ValueError, and a + # bare one of those does not say which sampler raised it. raise RuntimeError( - f"An error occurred in the 'reset_seed' method of " - f"{names[0]} CustomSampler" + f"An error occurred in the 'reset_seed' method of the " + f"CustomSampler for {', '.join(names)}" ) from error def _validate_custom_sampler(self, input_name, sampler): diff --git a/tests/unit/stochastic/test_custom_sampler.py b/tests/unit/stochastic/test_custom_sampler.py index 7d683e118..744087a9e 100644 --- a/tests/unit/stochastic/test_custom_sampler.py +++ b/tests/unit/stochastic/test_custom_sampler.py @@ -239,3 +239,53 @@ def test_an_independent_sampler_does_not_move_a_shared_group(): alongside["wind_x"], alongside["wind_y"], ) + + +class _RefusesTheSeed(_Gaussian): + """A sampler whose generator will not take the seed it is given. + + `numpy.random.RandomState` is the real case: it refuses anything above + 2**32-1 with a ValueError, and the seeds handed out here are 128 bits. + """ + + def __init__(self, mean, sd, failure): + super().__init__(mean, sd) + self.failure = failure + + def reset_seed(self, seed=None): + raise self.failure + + +@pytest.mark.parametrize( + "failure", + [ValueError("out of range"), TypeError("wrong type"), RuntimeError("boom")], + ids=lambda f: type(f).__name__, +) +def test_a_sampler_that_refuses_its_seed_is_named_in_the_error(failure): + """Only RuntimeError used to be caught, so a legacy RandomState sampler + raised a bare ValueError with nothing to say which input it came from.""" + with pytest.raises(RuntimeError, match="mass") as raised: + StochasticModel( + SimpleNamespace(mass=0.0), mass=_RefusesTheSeed(0.0, 1.0, failure) + ) + + assert raised.value.__cause__ is failure + + +def test_a_legacy_random_state_sampler_is_named_rather_than_raising_bare(): + """The concrete case, not a stand-in: RandomState really does refuse the + 128-bit seed this hands out.""" + + class LegacySampler(CustomSampler): + """Built on RandomState rather than default_rng.""" + + def sample(self, n_samples=1): + return list(self.rng.normal(size=n_samples)) + + def reset_seed(self, seed=None): + self.rng = np.random.RandomState(seed) + + with pytest.raises(RuntimeError, match="mass") as raised: + StochasticModel(SimpleNamespace(mass=0.0), mass=LegacySampler()) + + assert isinstance(raised.value.__cause__, ValueError) From 1e0e149cb552c715118f858f8ae3e462bec89b27 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 8 Aug 2026 15:07:33 +0800 Subject: [PATCH 5/8] MNT: reset the group itself, and stop the example filling a cache it 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> --- docs/user/custom_sampler.rst | 6 ++- rocketpy/stochastic/stochastic_model.py | 12 +++-- tests/unit/stochastic/test_custom_sampler.py | 57 ++++++++++++++++++++ 3 files changed, 70 insertions(+), 5 deletions(-) diff --git a/docs/user/custom_sampler.rst b/docs/user/custom_sampler.rst index 615ed7ea2..ab147a86b 100644 --- a/docs/user/custom_sampler.rst +++ b/docs/user/custom_sampler.rst @@ -243,14 +243,16 @@ below implements an example of such a generator """Reseeds the generator and discards the samples drawn before it The cached samples came from the previous generator, so keeping them - would let the first 1000 draws after a reseed ignore the new seed. + would let the first draws after a reseed ignore the new seed. Nothing + is generated here: ``top_up`` fills the shortfall when samples are + first asked for, and a reseed happens once per simulation, so filling + eagerly would build a thousand pairs to use one. """ self.rng = np.random.default_rng(seed) self.samples_list = [] self.samples_generated = 0 self.used_samples_x = 0 self.used_samples_y = 0 - self.generate_samples(1000) def top_up(self, used_samples, n_samples): """Generates enough samples to cover the request, if it is short""" diff --git a/rocketpy/stochastic/stochastic_model.py b/rocketpy/stochastic/stochastic_model.py index fd4ee84f4..f9a2ea16e 100644 --- a/rocketpy/stochastic/stochastic_model.py +++ b/rocketpy/stochastic/stochastic_model.py @@ -41,8 +41,10 @@ def _sampler_seed(seed, input_names): """ if isinstance(input_names, str): input_names = (input_names,) + # Sorted here rather than trusting the caller, so a future call site cannot + # give one group two different seeds by listing its members another way. root = np.random.SeedSequence( - entropy=seed, spawn_key=_names_as_spawn_key(tuple(input_names)) + entropy=seed, spawn_key=_names_as_spawn_key(tuple(sorted(input_names))) ) words = root.generate_state(4, dtype=np.uint32) return sum(int(word) << (32 * position) for position, word in enumerate(words)) @@ -507,9 +509,13 @@ def _reset_custom_samplers(self, seed): shared = groups.setdefault(id(group), ([], sampler, group)) shared[0].append(input_name) - for names, sampler, _group in groups.values(): + for names, sampler, group in groups.values(): + # The group itself when it can be reset, since it is the thing that + # holds the shared state. Going through one member instead assumes + # every member resets the same way and keeps nothing of its own. + resetter = group if hasattr(group, "reset_seed") else sampler try: - sampler.reset_seed(_sampler_seed(seed, names)) + resetter.reset_seed(_sampler_seed(seed, names)) except Exception as error: # Not just RuntimeError. The seed handed over is now 128 bits, # which the legacy RandomState refuses with a ValueError, and a diff --git a/tests/unit/stochastic/test_custom_sampler.py b/tests/unit/stochastic/test_custom_sampler.py index 744087a9e..5b176456e 100644 --- a/tests/unit/stochastic/test_custom_sampler.py +++ b/tests/unit/stochastic/test_custom_sampler.py @@ -289,3 +289,60 @@ def reset_seed(self, seed=None): StochasticModel(SimpleNamespace(mass=0.0), mass=LegacySampler()) assert isinstance(raised.value.__cause__, ValueError) + + +class _CountingShared(_SharedPair): + """Records how it was reset, so the dispatch can be checked.""" + + def __init__(self): + super().__init__() + self.reset_seed_calls = 0 + + def reset_seed(self, seed=None): + self.reset_seed_calls += 1 + self.reset(seed) + + +class _WrapperOverGroup(CustomSampler): + """A wrapper whose own reset_seed would be the wrong thing to call.""" + + def __init__(self, shared): + self.shared = shared + self.own_resets = 0 + + @property + def seed_group(self): + return self.shared + + def sample(self, n_samples=1): + return [self.shared.draw() for _ in range(n_samples)] + + def reset_seed(self, seed=None): + self.own_resets += 1 + self.shared.reset(seed) + + +def test_a_group_that_can_reset_itself_is_reset_directly(): + """Dispatching through one member assumes every member resets the same way + and holds no state of its own. The group owns the shared generator, so it + is the thing to reset when it knows how.""" + shared = _CountingShared() + first, second = _WrapperOverGroup(shared), _WrapperOverGroup(shared) + model = StochasticModel( + SimpleNamespace(wind_x=0.0, wind_y=0.0), wind_x=first, wind_y=second + ) + shared.reset_seed_calls = 0 + first.own_resets = second.own_resets = 0 + + model._set_stochastic(4242) + + assert shared.reset_seed_calls == 1 + assert (first.own_resets, second.own_resets) == (0, 0) + + +def test_a_group_key_does_not_depend_on_the_order_it_is_given(): + """The caller sorts today. The helper sorts too, so a future call site + cannot hand one group two different seeds by listing it another way.""" + assert _sampler_seed(4242, ("wind_x", "wind_y")) == _sampler_seed( + 4242, ("wind_y", "wind_x") + ) From cb5dca7291e5ae9b81f3d3f7490336c7db520ddc Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 8 Aug 2026 16:12:10 +0800 Subject: [PATCH 6/8] DOC: say what owning a seed group means 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> --- rocketpy/stochastic/custom_sampler.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/rocketpy/stochastic/custom_sampler.py b/rocketpy/stochastic/custom_sampler.py index 5b9b8598c..16cbfad6c 100644 --- a/rocketpy/stochastic/custom_sampler.py +++ b/rocketpy/stochastic/custom_sampler.py @@ -18,6 +18,14 @@ def seed_group(self): is seeded once as a unit rather than one of them silently overwriting the other's seed. + Return the same object on every call. Building the answer each time, + which a property invites, gives each member a different identity and + puts it back in a group of its own. + + A group belongs to one model. Declaring the same generator on two + models has them both seed it, and whichever is seeded last decides the + stream, which is the overwrite this is here to avoid. + Returns ------- object From e77584615da879ca37910f35f97c5ee9e21a4a0f Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 8 Aug 2026 17:22:45 +0800 Subject: [PATCH 7/8] DOC: add the changelog entry for this branch The automation that normally writes it cannot run on a pull request from a fork, which is #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> --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ab28dd89..432e9cd63 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,7 @@ Attention: The newest changes should be on top --> ### Fixed +- BUG: Give each `CustomSampler` input its own deterministic stream, and seed samplers sharing one generator once as a group. Existing fixed-seed `CustomSampler` baselines change, and samplers built on the legacy `RandomState` must move to `default_rng` because seeds now carry the full 128 bits. [#1102](https://github.com/RocketPy-Team/RocketPy/pull/1102) - BUG: rocket with a late-starting thrust curve never leaves the rail [#1085](https://github.com/RocketPy-Team/RocketPy/pull/1085) ## [v1.13.0] - 2026-07-21 From 23be0babade70f8f397c5c2850673eb16c3c4137 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 8 Aug 2026 21:22:50 +0800 Subject: [PATCH 8/8] Raise the sampler type check rather than asserting it Gui's point on the review. `python -O` strips an assert, and this is what keeps a non-sampler out of the model, so it has to be a raise. Same shape as #1103, which took the identical route for the parachute triggers. AssertionError is kept rather than swapped for TypeError, because the docstring on develop already documents it and a caller catching it should keep working. Two tests. One is the behaviour; the other runs a child interpreter under -O, since that is the mechanism and the plain test passes either way. Note this module carries thirteen more asserts on develop, none of them mine. Happy to send them separately if you want the same treatment there. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- rocketpy/stochastic/stochastic_model.py | 10 ++++-- tests/unit/stochastic/test_custom_sampler.py | 35 ++++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/rocketpy/stochastic/stochastic_model.py b/rocketpy/stochastic/stochastic_model.py index f9a2ea16e..5bb0598bf 100644 --- a/rocketpy/stochastic/stochastic_model.py +++ b/rocketpy/stochastic/stochastic_model.py @@ -545,9 +545,13 @@ def _validate_custom_sampler(self, input_name, sampler): AssertionError If the input is not in a valid format. """ - assert isinstance(sampler, CustomSampler), ( - f"`{input_name}` must be a CustomSampler, not {type(sampler).__name__}" - ) + # Raised rather than asserted, the same way #1103 handles it: `python -O` + # strips an assert, and the documented AssertionError is kept so callers + # that already catch it still do. + if not isinstance(sampler, CustomSampler): + raise AssertionError( + f"`{input_name}` must be a CustomSampler, not {type(sampler).__name__}" + ) return sampler def _validate_airfoil(self, airfoil): diff --git a/tests/unit/stochastic/test_custom_sampler.py b/tests/unit/stochastic/test_custom_sampler.py index 5b176456e..ae3d906ba 100644 --- a/tests/unit/stochastic/test_custom_sampler.py +++ b/tests/unit/stochastic/test_custom_sampler.py @@ -346,3 +346,38 @@ def test_a_group_key_does_not_depend_on_the_order_it_is_given(): assert _sampler_seed(4242, ("wind_x", "wind_y")) == _sampler_seed( 4242, ("wind_y", "wind_x") ) + + +def test_a_non_sampler_is_refused_even_under_optimisation(): + """`python -O` strips an assert, so the check that keeps a non-sampler out + of the model has to be a raise. The documented AssertionError is kept, so a + caller already catching it is unaffected.""" + model = StochasticModel(SimpleNamespace(mass=0.0)) + + with pytest.raises(AssertionError, match="must be a CustomSampler"): + model._validate_custom_sampler("mass", object()) + + +def test_the_refusal_survives_python_dash_o(): + """The mechanism, not just the behaviour: run it in a child with -O and + check the exception still arrives.""" + import subprocess + import sys + + program = ( + "from types import SimpleNamespace;" + "from rocketpy.stochastic.stochastic_model import StochasticModel;" + "m = StochasticModel(SimpleNamespace(mass=0.0));" + "\ntry:\n" + " m._validate_custom_sampler('mass', object())\n" + "except AssertionError:\n" + " print('refused')\n" + ) + done = subprocess.run( + [sys.executable, "-O", "-c", program], + capture_output=True, + text=True, + check=True, + ) + + assert "refused" in done.stdout, done.stderr