From 85d682d5a286f4e562496919fc543e071658c37a Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 15:51:43 +0800 Subject: [PATCH] DOC: tighten the comments that came with the sampler seed groups The prose added in #1102 restates itself across three places and narrates how each rule was arrived at. Keep the caller-visible rules, drop the retelling, and let a shared group's contract live on seed_group rather than being repeated at each call site. Comments only, no change to executable code. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- rocketpy/stochastic/custom_sampler.py | 23 +++++----------- rocketpy/stochastic/stochastic_model.py | 36 +++++++++---------------- 2 files changed, 19 insertions(+), 40 deletions(-) diff --git a/rocketpy/stochastic/custom_sampler.py b/rocketpy/stochastic/custom_sampler.py index 16cbfad6c..a29b8041d 100644 --- a/rocketpy/stochastic/custom_sampler.py +++ b/rocketpy/stochastic/custom_sampler.py @@ -10,27 +10,18 @@ class CustomSampler(ABC): @property def seed_group(self): - """The generator state this sampler shares, if it shares one. + """The generator this sampler shares, or ``self`` if it shares none. - 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. - - 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. + Samplers sharing a generator must all return it, so the group is seeded + once rather than each member overwriting the previous seed. Return the + same object every call: a rebuilt one has a new identity and forms a + group of its own. A group belongs to one model; declared on two models, + each seeds it and the last one wins. Returns ------- object - Identity is what counts, not equality. ``self`` by default, which - makes every sampler its own group. + Matched by identity, not equality. Defaults to ``self``. """ return self diff --git a/rocketpy/stochastic/stochastic_model.py b/rocketpy/stochastic/stochastic_model.py index 79829beff..6144168f2 100644 --- a/rocketpy/stochastic/stochastic_model.py +++ b/rocketpy/stochastic/stochastic_model.py @@ -585,39 +585,30 @@ def _validate_positive_int_list(self, input_name, input_value): ) def _reset_custom_samplers(self, seed): - """Give each sampler its own stream, and each shared group one between - them. + """Seed each sampler, and each shared generator once. - 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. + Kept out of the validation loop in ``_set_stochastic``, whose order + sets ``__dict__`` and with it the order every other input is drawn in. """ groups = {} for input_name in sorted(self.__stochastic_dict): sampler = self.__stochastic_dict[input_name] 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. + # Kept in the value too: `id` is unique only among live + # objects, so the group has to outlive the dict. group = sampler.seed_group shared = groups.setdefault(id(group), ([], sampler, group)) shared[0].append(input_name) 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. + # The group holds the shared state, so reset it directly; a member + # may reset differently, or keep state of its own. resetter = group if hasattr(group, "reset_seed") else sampler try: 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 - # bare one of those does not say which sampler raised it. + # Broad: the seed is 128 bits, which legacy RandomState refuses + # with a ValueError that does not name the sampler. raise RuntimeError( f"An error occurred in the 'reset_seed' method of the " f"CustomSampler for {', '.join(names)}" @@ -627,9 +618,7 @@ 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. + Seeding happens in ``_reset_custom_samplers``, not here. Parameters ---------- @@ -643,9 +632,8 @@ def _validate_custom_sampler(self, input_name, sampler): AssertionError If the input is not in a valid format. """ - # 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. + # Raised, not asserted: `python -O` strips asserts. AssertionError is + # kept so callers that catch it still do. Same as #1103. if not isinstance(sampler, CustomSampler): raise AssertionError( f"`{input_name}` must be a CustomSampler, not {type(sampler).__name__}"