Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend RNG options to three, addressing issue #348 #349

Merged
merged 16 commits into from
Mar 18, 2024
Merged

Extend RNG options to three, addressing issue #348 #349

merged 16 commits into from
Mar 18, 2024

Conversation

daniel-klein
Copy link
Contributor

@daniel-klein daniel-klein commented Mar 2, 2024

Renames and extends the multirng option in settings, now called 'rng'.

Set how random numbers are handled in Starsim with three options:

  • "centralized" uses the centralized numpy random number generator for all distributions.
  • "single" uses a separate (SingleRNG) random number generator for each distribution.
  • "multi" uses a separate (MultiRNG) random number generator for each distribution.

"single" is the default value for this option.

In comparing two simulations, the "single" option may be slightly better than the "centralized" option without any real disadvantages. Only "multi" can achieve full common random number (CRN) coherence, but is more computationally expensive and only produces CRN results when using CRN-safe code.

In practice, devtest_rng.py shows that multi > single > centralized, but that single and centralized have nearly identical performance. So this PR switches the default to single, which uses the SingleRNG under the hood to track seeds for each stream.

Closes #348

Copy link
Contributor

@cliffckerr cliffckerr left a comment

Choose a reason for hiding this comment

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

This works, but I'm not sure why we would want to give users one good option (multi-RNG) and two bad options (single and centralized)? I could imagine someone using single instead of multi if they only care about performance and their own code isn't RNG-safe, but I don't know why someone would want centralized instead of single.

Suggest we consider this together with #351 (once it's ready) and decide how to proceed.

From benchmark_full.py, I got:

'centralized': 3451 ± 19 ms
'single':      3362 ± 13 ms
'multi':       4851 ± 37 ms

@daniel-klein
Copy link
Contributor Author

Unclear to me if most users will want 'single' or 'multi'. We currently only have one network (EmbeddingNet) and a few diseases that are CRN safe. It makes sense to run 'multi' for these, depending on the application, but otherwise users will use the default 'single' option.

I do want to keep 'centralized' around, e.g. to create figures for the CRN paper. But it can be obscure (sorta already is?) and likely removed at a later time.

@daniel-klein
Copy link
Contributor Author

What do you think about the names?

I like 'centralized', that's clear as to what's going on. But 'single' vs 'multi' is a misnomer considering both establish one random number generator instance per distribution. Single could be 'per-distribution' or even 'multi'? And what's currently 'multi' could become 'CRN'? Thoughts?

@daniel-klein
Copy link
Contributor Author

Of course, if we rename 'multi', I would also change the name of the 'MultiRNG' and we'd have to fix all the 'options.multirng' calls, but that's doable. And 'SingleRNG' could stand to be renamed as part of that as well.

@cliffckerr
Copy link
Contributor

Ok, we can merge for now and refactor later. I agree the names aren't perfect, but would suggest holding off on a refactor until we decide on the long-term path forward.

@cliffckerr cliffckerr self-requested a review March 2, 2024 19:36
Copy link
Contributor

@cliffckerr cliffckerr left a comment

Choose a reason for hiding this comment

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

Almost the entirety of SingleRNG's code is duplicated in MultiRNG:
image
To reduce code duplication, could SingleRNG be the base class and then MultiRNG derive from it?

@daniel-klein daniel-klein changed the title Fix #348 Extend RNG options to three, addressing issue #348 Mar 9, 2024
Copy link
Contributor

@cliffckerr cliffckerr left a comment

Choose a reason for hiding this comment

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

Let's merge this and then compare with refactor-rngs.

@cliffckerr cliffckerr self-requested a review March 18, 2024 14:01
Copy link
Contributor

@cliffckerr cliffckerr left a comment

Choose a reason for hiding this comment

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

Looks good, will merge for now

@cliffckerr cliffckerr merged commit 94ad0f1 into main Mar 18, 2024
2 of 3 checks passed
@cliffckerr cliffckerr deleted the fix_348 branch March 18, 2024 14:02
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.

Three options for rng
2 participants