Skip to content

DOC: make the CustomSampler examples answer to their seed - #1097

Merged
Gui-FernandesBR merged 1 commit into
RocketPy-Team:developfrom
thc1006:doc/custom-sampler-seed
Aug 8, 2026
Merged

DOC: make the CustomSampler examples answer to their seed#1097
Gui-FernandesBR merged 1 commit into
RocketPy-Team:developfrom
thc1006:doc/custom-sampler-seed

Conversation

@thc1006

@thc1006 thc1006 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Closes #1096.

Pull request type

  • ReadMe, Docs and GitHub updates

Checklist

  • Docs have been reviewed and added / updated

The other three items are removed rather than ticked. Nothing under rocketpy/ or tests/ changes, so the suite and the linters have nothing to say about this branch and I did not run them on it. What I did run is the documentation build and the code the page contains, both below.

Current behavior

Both CustomSampler examples build a generator and discard it:

def reset_seed(self, seed=None):
    np.random.default_rng(seed)

Nothing is assigned, so the call has no effect, and sample goes on drawing from the process-global np.random. A sampler written by following this page ignores random_seed entirely. The failure is quiet: the study runs, the numbers look reasonable, and only a second run with the same seed shows they were never reproducible. Nothing in RocketPy seeds the global RNG, so there is no accident that would rescue it.

The bivariate generator has two further defects, found while fixing the first.

It caches 1000 samples up front. Even with reset_seed corrected, those samples came from the generator that was replaced, so the first 1000 draws after a reseed would still ignore the new seed.

Its refill test is

if self.samples_generated < self.used_samples_x:
    self.generate_samples(n_samples)

which only becomes true once the cache has already run out, and the slice that follows is not bounded either. A study longer than the cache silently receives short lists:

draw 1..1000   sample(n_samples=1) -> 1 value
draw 1001      sample(n_samples=1) -> []
               dict_generator does sample(...)[0] -> IndexError

New behavior

reset_seed keeps the generator, sample draws from it, and __init__ routes through reset_seed so construction and reseeding agree. For the bivariate generator, reset_seed also drops the cache, and the refill covers the shortfall rather than reacting after the fact.

Executing the code blocks straight out of the page, before and after:

                                          before        after
mixture, same seed twice                  differs       same
bivariate, same seed twice                differs       same
1501 draws of one sample, short returns   501           0
wind X against wind Y                     IndexError    0.6986

The last row is the property the shared generator exists for. The measured correlation matches 0.171/sqrt(0.2*0.3) = 0.6981, so splitting the seeding did not split the two wind components.

I ran those checks by parsing the jupyter-execute blocks out of the .rst and executing them, rather than retyping the classes, so what passed is what the page says.

The added .. warning:: is there because the mistake is easy to repeat and produces no error when repeated.

Breaking change

  • No

Documentation only. No module in rocketpy/ is touched.

Additional information

The same mistake was in tests/fixtures/monte_carlo/custom_sampler_fixtures.py and is fixed in #1054. This uses the same corrected shape so the two do not drift apart. It is independent of that PR and can go in first or second.

Sphinx built clean with the CI settings (-W --keep-going, DOCS_SKIP_EXECUTE=1): no warning from custom_sampler.rst, and the rendered page contains the corrected code with no global draws left.

Related: #1093, #1094, #1095, filed from the same review.

Both examples build a generator with np.random.default_rng(seed) and drop it,
then sample from the process-global np.random. reset_seed is a no-op, so a
sampler written by following this page ignores random_seed. The study runs and
the numbers look reasonable; only a second run with the same seed shows they
were never reproducible.

The bivariate generator needs two more things. It caches 1000 samples, so
reset_seed has to discard them or the first 1000 draws after a reseed still
come from the generator that was replaced. And its refill test compares
samples_generated against used_samples, which only becomes true after the
cache has run out, so a study longer than the cache silently got short lists:
sample(n_samples=1) returns [] at simulation 1001 and dict_generator raises
IndexError on [0].

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
@thc1006
thc1006 requested a review from a team as a code owner August 8, 2026 00:32
@thc1006

thc1006 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Worth knowing when reviewing this one: the Documentation workflow is scoped to branches: [master], so it does not run here. A docs change merged into develop gets no build check until it reaches master, which means my local run is the only thing standing behind the reStructuredText.

For the record, that run was sphinx-build -b html -W --keep-going with DOCS_SKIP_EXECUTE=1 and MPLBACKEND=Agg, the same settings as the workflow. No warning from custom_sampler.rst, the page rendered, and the rendered HTML contains the corrected code with no global draws left in it.

Happy to open a separate issue about the branch filter if you think docs PRs onto develop should be building. It is a one-line change to the workflow and not something I would fold into this PR.

@Gui-FernandesBR
Gui-FernandesBR merged commit 2cca437 into RocketPy-Team:develop Aug 8, 2026
1 check passed
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.

CustomSampler documentation teaches a reset_seed that does nothing

2 participants