Skip to content

feat: Modify inter-shot random_seed behaviour, provide legacy behaviour through seed_mode - #209

Open
jake-arkinstall wants to merge 6 commits into
0.3-seriesfrom
feat/seed-mode
Open

feat: Modify inter-shot random_seed behaviour, provide legacy behaviour through seed_mode#209
jake-arkinstall wants to merge 6 commits into
0.3-seriesfrom
feat/seed-mode

Conversation

@jake-arkinstall

Copy link
Copy Markdown
Collaborator

Adjusts the random_seed behaviour to avoid overlap between batches of run_shots using nearby seeds.

The existing behaviour was such that if one ran run_shots of N shots with a random_seed of M, components are seeded with M on the first shot, M+1 on the second, up to M+N-1. This acts as a reproducibility aid, but causes confusion when expected entirely different batches of shots.

The new default behaviour is to randomise the seed per-shot, by passing random_seed to a Pcg64Mcg pRNG and yielding shot RNGs by advancing this RNG to the required shot. As such, the chance of any overlap between two subsequent run_shots runs is very small, even if the seeds are close.

The existing behaviour still serves some purposes, so is available via a new seed_mode parameter that can be provided to run_shots and run. Pass "legacy" for the existing mode, "default" for the new randomised mode. We demonstrate this by persisting some original snapshots with this mode in this PR, but may revise them to use the default mode later on.

The legacy mode will be supported throughout this major version. It will eventually be dropped entirely, and our only mode will be to randomise seeds between shots. To repeat prior experiments, shot_increment and shot_offset can be used as demonstrated in determinism tests.

… seeds a pRNG before yielding seeds per-shot
@jake-arkinstall

Copy link
Copy Markdown
Collaborator Author

Some guidance for reviewers.

  • The test/resources directory is just generated QIS. Some files were added when regenerating some guppy code for different seed_mode tests, but are widely irrelevant for the purposes of review.

  • Notice that traces and result snapshots are unchanged compared to the original snapshots when giving them seed_mode="legacy". Note that some have simply moved when pytest.mark.parametrizing them over seed mode.

  • I am not convinced that legacy and default are the right names, as they aren't descriptive, but as the legacy mode is to be dropped later on it seems reasonable enough.

  • Note that a new test has been added comparing the seed modes. It is arguably the most important aspect of this PR to review.

@jake-arkinstall
jake-arkinstall marked this pull request as ready for review August 18, 2026 12:52
Copilot AI lite review requested due to automatic review settings August 18, 2026 12:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

@cqc-alec
cqc-alec self-requested a review August 18, 2026 12:55
Comment thread selene-sim/python/selene_sim/instance.py Outdated
Comment thread selene-sim/python/selene_sim/instance.py Outdated
Comment thread selene-sim/python/selene_sim/instance.py Outdated
Comment thread selene-sim/python/selene_sim/instance.py Outdated
Setting to True provides the high level Selene interface, and
using False allows for Selene to be used as an intermediate
component for use with an external result stream handler.
seed_mode: The mode for handling random seeds.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth considering using an enum for this instead of a string?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be. Though it will necessitate an extra import e.g.:

from selene_sim import SeedMode

run_shots(..., seed_mode=SeedMode.legacy)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, perhaps not worth it.

Comment thread selene-sim/rust/selene_instance/configuration.rs Outdated
jake-arkinstall and others added 5 commits August 18, 2026 15:47
Co-authored-by: Alec Edgington <54802828+cqc-alec@users.noreply.github.com>
Co-authored-by: Alec Edgington <54802828+cqc-alec@users.noreply.github.com>
Co-authored-by: Alec Edgington <54802828+cqc-alec@users.noreply.github.com>
Co-authored-by: Alec Edgington <54802828+cqc-alec@users.noreply.github.com>
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.

3 participants