ENH: continue a Monte Carlo study from the root its rows were drawn with

[thc1006]

A Monte Carlo run cannot be seeded on develop: simulate() takes no random_seed. Seeding it per simulation index is not enough on its own, because an append then derives a fresh root and writes it into the same file, so a study resumed after a restart holds two lineages with nothing afterwards to say which simulation came from which. Both halves are here. Addresses #1053 and #1075.

This branch carries #1054's work as well, which is where the root comes from. That pull request is closed in favour of this one: its per-index seeding is here in full, with the append lineage built on top. The branch is rebased onto 88aed5a5 and is five commits: three mine, two @Gui-FernandesBR added when he resolved the rebase onto #1177. The first of mine is 353 lines of monte_carlo.py, three test files of its own, a changelog line, a note in the stochastic guide, three added lines in the Monte Carlo notebook, and 71 lines of #1182's worker tests moved onto the claim this replaces; the other two are 18 lines of documentation and one test. His carry #1177's interrupt tests onto the same loop and add a Changed changelog entry. Together the diff is ten files, 1331 insertions and 71 deletions.

Pull request type

• Code changes (bugfix, features)

Checklist

• Tests for the changes have been added (if needed)
• Docs have been reviewed and added / updated
• Lint (ruff check / ruff format --check, pylint) has passed locally
• All tests have passed locally, other than ten that need optional dependencies this machine does not have. They fail the same way on develop, listed below.
• CHANGELOG.md: one line, since one pull request is one line here and 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

On develop there is no seed to carry: random_seed does not appear in monte_carlo.py at all, so a run cannot be fixed and an append continues an unseeded one.

Per-simulation-index seeding, which was #1054 and is carried here, is not enough by itself. With it in place and nothing else, append=True derives a fresh root and carries on writing into the file it was given. Nothing records which root produced which row, so the file ends up describing two studies with neither tellable from the other. That is what #1075 is filed against.

New behavior

simulate() takes a random_seed, and each simulation derives its own from its index rather than from the worker that happens to run it. A serial run and a run split over any number of workers draw the same inputs for the same index, and the serial path numbers its simulations from zero the way the parallel one always did, so the two no longer give one simulation two names.

Every input row carries the root that drew it. An append reads it back and continues from it, so resuming needs no seed, and a fresh object over the same files is the ordinary way to do it. A seed that disagrees with the rows is refused rather than mixed in. A log whose rows disagree with each other is refused rather than resolved, since that is two studies already and continuing either buries the other.

Rows that carry no root at all are refused as well, rather than read as an empty log. That is how a study written before this release looks, and taking it for an empty one starts the second lineage this exists to prevent. A genuinely empty log still starts normally, which has a test of its own.

Why the root is in the rows

The obvious alternative is a small file written beside the log. I built that first, in the branch this came out of, and it grew four problems with one root between them: a sidecar cannot be shown to belong to a log.

Names and counts match by coincidence. A copy taken from another study passes every check that reads only itself. The write that claims ownership has to be transactional with the log replacement. And the count it carries can drift from the rows it describes.

Putting the root in the rows removes the first two rather than solving them: there is no ownership write to make atomic and no count that can drift, because there is no second file. The other two become questions about the rows themselves, which is where they can be answered. The root a row records has to be one a stream can be rebuilt from, and the two logs have to name the same study; both are checked before an append writes anything. The cost is one field per row, in rows that already carry a serialized rocket.

What this does not do

It reads both logs. Every output row carries a digest of the root its input row records, so an output log from another study is refused even when its indices match, and an append will not start until the two agree on the simulations they hold. What it does not do is judge a finished run: whether every simulation the run was asked for reached the logs is the completeness check that landed with #1182, which this branch now sits on top of.

It does not repair a log with a hole in it. A run that lost simulations is refused, not filled in.

Two things I have deliberately left, and would rather not decide alone on a format people will have on disk: versioning the persisted root, and representing a wide entropy losslessly for readers that parse JSON numbers as doubles.

Breaking change

• Yes

Two of them, both visible to code that reads a study off disk.

A serial run numbers its simulations from zero, as the parallel path already did. The two used to name the same simulation 1, 2, 3 and 0, 1, 2, so the index field in the inputs and outputs rows shifts by one for a serial run: n simulations are numbered 0 to n - 1 whichever way the run was started.

append=True onto a log written before this release is refused, because such a log carries no root and cannot be shown to be one study. Re-running it writes a log this release can continue.

Coverage

The patch showed six lines uncovered. Four belong to the append and now have tests: a
sequence seed reaching the row whole, a blank line not being a row, and a row that
will not parse being refused rather than read as an empty log. The other two came in
with the seeding work and their tests came with it: one that the serial failure
message names the same index the parallel path would, and one that drives the worker
loop in this process rather than a child. All six are on this branch now.

The documentation change is prose, inside one .. note:: and one docstring. The note
points at the custom sampler page with a :ref: that page already defines and
stochastic.rst already uses once, and build-docs is green on it.

Additional information

Verification, on a clean tree:

pytest tests/                          2619 passed, 10 failed
  the same on develop at 88aed5a5      2557 passed, 10 failed
pytest rocketpy --doctest-modules        48 passed
ruff check . / ruff format --check .    clean
pylint rocketpy/ tests/ docs/           10.00/10, exit 0

The ten are the same ten on both, which is why I ran develop at the commit this branch sits on, 88aed5a5. Each of them is an optional dependency this machine does not have: statsmodels and prettytable for the sensitivity tests, imageio for the ellipses one, and timezonefinder, windrose, ipywidgets and jsonpickle for the environment analysis ones.

Each mechanism is pinned by a mutation:

undone	goes red
rows stop carrying the root	33
the append stops reading it back	2
disagreeing rows pick the first instead of refusing	1
a rootless log is read as an empty one	2
a sequence seed does not reach the row	2
blank lines are counted as rows	2
a row that will not parse is let through	1
the nominal is read again on each reseed	1, and the constant-draw control survives

Counts are from the branch as it stands on 88aed5a5, over
test_append_lineage.py and test_monte_carlo_simulation_index.py, where the
control is 43 passed. They were measured again after each rebase rather than
carried forward: #1182, #1129, #1177 and #1137 have landed under this branch
since the table was first written, and one of the anchors stopped being unique
when #1182 brought a second blank-line skip into the file, so that row was
re-measured on its own against the one in _what_the_rows_say_drew_them. The
last row is measured separately, since undoing it means taking #1169's kept
nominal away rather than editing this branch. Every mutation is reverted from
git show HEAD: rather than from a copy, after one run left a file mutated and
the next took that as its baseline.

Bringing develop in was more than a conflict fix. #1169 landing made this branch's documentation note false: it said a run moves the nominal as it goes, so a flight could still differ by which worker took an index, and closing that is what #1169 did. The four cases it leaves outside the rule are decided by the index too, so the note is rewritten and moved to the end of the component seeding section that #1170 added. The existing split test could not have caught it, since every stochastic input in its fixture is given an explicit nominal and nothing reads one off the wrapped object. The new test uses a bare standard deviation, which is the form that does. It measures index 3 at an elevation of 1400.221102854623 whether it is reached through 0, 1 and 2 or taken on its own. Take the kept nominal away and the two stop agreeing, 1367.1672115143288 against 1367.3883143689518.

A fifth commit takes the wall clock out of one of #1182's join tests. It is on develop rather than in this branch, and it is mine: the stand-in workers return at once, so nothing makes the clock move between the shared deadline being set and being read back for the last of them, and (t + 0.05) - t is not exactly 0.05 in binary. It went red on windows-latest 3.10 at 0.0500000000001819 while every other leg passed. The clock is a counter now and the shape the test is about is asserted as well. It is here because it blocks this pull request's CI, and I am happy to move it out.

#1177 landing turned up the same thing from the other side, and @Gui-FernandesBR resolved it rather than leaving the branch conflicted. Its interrupt tests stand a double in for MonteCarlo, and the double stubs only what __run_in_serial reached: the loop now reseeds per index, so the double had nothing to reseed, and its rows carried no root, so the append test tripped the guard for logs written before that check instead of exercising the continuation. His two commits carry those tests over and add a Changed changelog entry for the two user-visible breaks. I checked both against the source rather than the diff: develop really did number serial runs from one and parallel from zero, and the error-file append he factored out was a no-op for an empty row either way, since __setup_files opens that file w+ or r+ before either handler can run.

#1182 landing turned up a second one that neither branch could see alone. Its worker tests drive __sim_producer with a stand-in monitor, and that stand-in offers keep_simulating() and increment(), which this branch replaces with one claim; its _run also passes the per-worker seed the producer no longer takes. Nineteen of them went red on the rebase. They move to the claim here, and one of them changes meaning rather than shape: seeding runs after the claim now instead of once above the loop, so a model that will not reseed is reported against the index it was seeding for rather than against worker startup. The test that covered the old wording asserts the new one, and a separate test still covers a failure before any index is claimed.

Three things came out of reviewing this against itself, and all three are in:

• a log whose rows carry no root was read as empty, so an append onto one from an earlier release produced exactly the two lineages in one file that this is for. Measured before the fix: four rows, two distinct roots.
• the check held one record per row in memory to compare them. It reads one at a time now and compares with the first.
• it ran before the working-log refusal, so a .csv was reported as a row that could not be read rather than as a format simulate cannot use. BUG: refuse to run a Monte Carlo over a results file it cannot write #1161's message goes first now.

[thc1006]

cc @zuorenchen, since this sits next to your stochastic_flight work.

Updating this, because what it originally said has been overtaken. The series it listed has landed: #1181, #1169, #1170 and #1174 are on develop, and #1054 is closed in favour of this branch, which carries its per-simulation-index seeding in full. This one and #1182 are what is left.

What it does: an append continues the root its rows were drawn with, rather than quietly starting a second one in the same file.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.58%. Comparing base (ebe1b77) to head (65fda0c).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1187      +/-   ##
===========================================
+ Coverage    91.51%   91.58%   +0.07%     
===========================================
  Files          132      132              
  Lines        18305    18404      +99     
===========================================
+ Hits         16751    16855     +104     
+ Misses        1554     1549       -5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Gui-FernandesBR]

@thc1006 conflicts must be solved before reviewing

have you tired stacked PRs before? See: https://docs.github.com/en/pull-requests/how-tos/stacked-pull-requests

[thc1006]

Conflicts are gone. The branch is squashed to one commit and rebased onto current develop, so the history here is one change rather than twenty plus two merges of develop.

On the stacking, since you asked: both of mine were stacked and neither is now. #1182 sat on #1181, which landed. This one sat on #1054, and I closed #1054 rather than keep the chain going, because this branch is a superset of it. The per-index seeding is here in full and the append lineage sits on top of it. Both target develop directly now.

The merge was not mechanical, so one thing is worth flagging. #1169 landing made a sentence in this branch's documentation false. The note said a run moves the nominal as it goes, so a flight could still differ by which worker took an index. That is exactly what #1169 closed, and the four cases it leaves outside the rule turn out to be decided by the index too: a component position is read from a component the run never writes back to, and select_ensemble_member rebuilds the wind from the member's own arrays rather than scaling what is already there. I rewrote the note and moved it to the end of the component seeding section, which #1170 closes by saying per-simulation reseeding is what this work adds.

The existing split test could not have caught that. Every stochastic input in its fixture is given an explicit nominal, so nothing reads one off the wrapped object, and taking the kept nominal away leaves all five of those tests green. I added one that uses a bare standard deviation, which is the form that does read one, with a control that survives.

The numbers are the test's own, so they are reproducible by running it: index 3 draws an elevation of 1400.221102854623 whether it is reached through 0, 1 and 2 or taken on its own, and taking the kept nominal away leaves 1367.1672115143288 against 1367.3883143689518. The commit message quotes a standalone probe with a nominal of 1000 instead, which is where 1002.211028546 and 671.672115143 come from.

[Gui-FernandesBR]

Heads up @thc1006 — I rebased this branch onto develop and force-pushed, so please git fetch and reset your local copy before you push again. Your commit is preserved, authorship and all; I added two on top. Sorry for rewriting under you — the alternative was leaving the PR conflicted.

The conflict was one hunk in __run_in_serial. Both sides bind a name before the try so the handlers cannot meet it unbound: #1177 binds inputs_json for the KeyboardInterrupt handler, your branch binds sim_idx for the Exception one. I kept both — dropping either puts back the UnboundLocalError the other side had just removed.

Keeping both pushed the function to 27 statements, two over max-statements, which would have failed lint. So the error-file append that the two handlers had copies of moved into __record_failed_inputs. The if inputs_json: guard travelled with it: the error file is created in __setup_files, so appending "" was already a no-op and the guarded and unguarded copies did the same thing.

What the textual merge could not show is that #1177's interrupt tests were written against the loop you replace. Four things had to be carried over:

• _InterruptingMonteCarlo stubs only what __run_in_serial touched, and the loop now also calls __seed_this_simulation, which reads models the double has none of. Stubbed like its other name-mangled members — seeding is pinned by your test_monte_carlo_seeding.py.
• The double's rows carried no run_root, so the append test tripped your guard for studies written before that check existed instead of exercising the continuation. It now writes the root the way the real builders do: the inputs file whole, the outputs file by digest.
• _SimMonitor.keep_simulating is now claim_next_index, and one test monkeypatches it by name.
• Serial numbering now starts at zero, so three tests reading indices off disk expected 1, 2, 3 where a run writes 0, 1, 2.

The last two are your intended change, pinned by your own test_monte_carlo_simulation_index.py — I only carried the stale expectations over, and touched no behaviour on either side.

One addition worth your review: the changelog entry was under Added only, and two things here are visible to someone whose code already reads a study off disk — the serial index shifting by one, and an append onto an older study now being refused. I added a Changed entry for both. Correct it if I have mischaracterised either.

Verified locally before pushing: tests/unit/simulation/ 322 passed, 8 skipped; ruff clean; pylint 10.00. Baseline check first — the eight failures were from the combination, not from the resolution: those tests are green on plain develop.

[thc1006]

Thanks for doing that, and no apology needed. Resetting onto it was the right call over leaving it conflicted.

I checked the two things you asked about and both hold.

The Changed entry is right on both counts. I read 88aed5a5 to be sure rather than take it from the diff: serial ran increment() and then passed sim_monitor.count, so the first simulation was 1, while the parallel loop took increment() - 1 and started at 0. So the two really did name the same simulation differently, and the shift is one for serial only. The refused append is the other half of it, and Changed is where #1129 put its own unpacking break, so that is the right section.

The __record_failed_inputs extraction is behaviour-preserving. The Exception handler's copy was unguarded, so the question was whether open(..., "a") plus write("") could ever create a file the guarded version leaves alone. It cannot get the chance: __setup_files opens the error file w+ for a fresh run and r+ for an append, so by the time either handler can run it exists. Measured on this branch, the file is unchanged after the guarded call and after the old unguarded write, and the two only diverge on a missing file, which __setup_files never leaves behind.

The four carried-over test changes read correctly to me, and the run confirms it.

Two things from my side.

#1137 landed after your rebase and conflicted again, on one line: it dropped the too-many-statements disable from __sim_producer, and this branch drops the seed parameter from the same signature. Resolved to the seedless signature with the disable left off, and pylint still comes out at 10.00 with exit 0. Correcting myself on why, since I first wrote that the seed going from the signature was what did it: max-statements counts the body, not the parameters, and putting the seed back leaves too-many-statements silent. What shortened the body is the seeding. develop reseeded three models above the loop and spent two statements claiming an index; this reseeds once inside the loop and claims in the while condition, which is three statements fewer. I rebased all three commits onto 88aed5a5; yours are unchanged apart from the new base.

The rebase also dropped one line from my commit message, so the paragraph about #1182's worker tests started mid-sentence on move to the claim along with it. I put the subject back. Nothing else in the message changed.

Verified here on 88aed5a5:

pytest tests/                       2619 passed, 10 failed
  the same on develop at 88aed5a5   2557 passed, 10 failed
ruff check . / ruff format --check  clean
pylint rocketpy/ tests/ docs/       10.00/10, exit 0

The ten are the same ten on both, all optional dependencies this machine does not have.

[thc1006]

Two commits on top, and one of them is about the windows-latest 3.10 leg that went red on dee8438b.

It is not a flake in the usual sense, and it is not this branch's. The failure is

assert min(offered) < 0.05
E   assert 0.0500000000001819 < 0.05

in test_the_fleet_comes_down_on_one_deadline_not_one_each, which is on develop and is mine, from #1182. _wait_for_the_workers sets deadline = monotonic() + seconds and offers each worker deadline - monotonic(). The stand-ins return at once, so nothing makes the clock move across the five of them, and (t + 0.05) - t is not exactly 0.05 in binary: it read 0.0500000000001819, a hair over the bound rather than under it. Linux and macOS tick between the calls and land at 0.049997, which is why only that one leg failed, and why 3.14 on the same runner passed.

The clock is a counter in that test now, so what each worker is offered is decided by arithmetic rather than by the platform's timer resolution. I also asserted the shape it is actually about: inside one stage the offers shrink along the fleet, where one deadline each would hand every worker the whole grace. Giving each worker its own grace turns both assertions red and leaves the other twelve in the file green.

It is a test-only change to a file this branch does not otherwise touch, and it is here because it blocks this pull request's CI. Glad to move it to its own pull request if you would rather keep them apart, and it would be worth landing either way, since it will keep catching Windows runners.

The other commit came out of reading your Changed entry. You were right that the serial renumbering is user-visible, and checking it turned up that the changelog was the only place it was written down. The guide's note leans on it without saying so: it claims simulation 7 draws the same inputs serially or split over workers, which is only one claim if both paths agree on which simulation 7 is. That note now says a run of n numbers them 0 to n - 1, and the notebook says it where a reader meets simulate(). I left the notebook's stored outputs alone: nbsphinx never re-executes them, and a seeding change moves every number in them, which is not a diff worth reading.

Verified on 88aed5a5:

pytest tests/                       2619 passed, 10 failed
  the same on develop at 88aed5a5   2557 passed, 10 failed
ruff check . / ruff format --check  clean
pylint rocketpy/ tests/ docs/       10.00/10, exit 0

The ten are the same ten on both, all optional dependencies this machine does not have.

[Gui-FernandesBR]

Rebased onto develop again @thc1006 — #1132 and #920 landed since, so please git fetch and reset before your next push. Sorry for moving the ground under you a second time.

Only CHANGELOG.md conflicted this time, add/add at the top of Added against #920's entry. Both kept, ours on top per "newest on top". Your two commits came through untouched.

Your TST: take the wall clock out of the shared-deadline test is the right fix, and I can confirm it works where it matters: this machine reproduced that failure 3/3 deterministically on plain develop, and the file is now 13/13 green across three runs. Ticking the clock by hand beats loosening the comparison, and the last < first assertion you added actually pins the shared-deadline property the test is named after — the old one would have passed with a deadline per worker on a coarse clock.

Full tests/unit/simulation/ on the rebased branch: 333 passed, 6 skipped, nothing failing. ruff clean, pylint 10.00.

[thc1006]

Thanks for rebasing it twice, and for checking the clock fix on your own machine rather than taking my word for it. Reproducing the failure 3/3 on plain develop is more than I managed here, and you are right that the old assertion would have passed with a deadline per worker on a coarse timer. That is the part I had missed when I wrote it in #1182.

TL;DR: I pushed once more. Nothing in the tree changed, the two tree hashes are the same. Two of my commit messages are reworded so that no line in them starts with #, because one paragraph of mine has now gone missing twice and a second one was set up to go the same way.

What happened to the message. My first commit had a paragraph beginning #1182's worker tests drive the producer.... After your first rebase that line was gone and the paragraph started mid-sentence on move to the claim along with it.. I put it back. After your second rebase it was gone again.

I could not pin down which step drops it, and I would rather say so than guess: I tried a conflicted rebase resolved with git rebase --continue in a scratch repository and the line survived, so whatever removes it is not that on its own. What I can show is that git has a mode which removes such lines, and that it is the one git picks when a message goes through an editor:

--cleanup=whitespace   line kept
--cleanup=verbatim     line kept
--cleanup=default      line kept
--cleanup=strip        line removed

So a line starting with # is fragile in any flow that reopens the message, wherever that happens to be. Rather than restore it a third time I reworded both paragraphs so no line begins with #. The second one had not bitten yet: my TST: take the wall clock out of the shared-deadline test had #1182. at the start of a line and would have lost it on the next rebase.

Please do not read this as a complaint about the rebases. The fragile thing was in my message, and it is gone now.

On the approval. It is against e339980e and the head has moved three times since, so it no longer covers what is there, and this push is not what changed that. The push is text only: no file, test or behaviour differs, and your two commits come through byte for byte, message, author and date.

CI agrees with your machine, for what it is worth as a second data point: Pytest (windows-latest, 3.10) was the one red leg on dee8438b and is green on 65fda0cd, which is the first head to carry the fix. All nine checks passed there.

Verified here on ebe1b775:

pytest tests/                        2644 passed, 10 failed
  the same on develop                2582 passed, 10 failed
ruff check . / ruff format --check   clean
pylint rocketpy/ tests/ docs/        10.00/10, exit 0

The failures are the same set on both, all of them optional dependencies this machine does not have.