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

Don't restore the random number generator state by default. #181

Merged
merged 3 commits into from
Jan 29, 2024
Merged

Conversation

plietar
Copy link
Member

@plietar plietar commented Jan 26, 2024

It can have suprising side effects by resetting the user global RNG state to a fixed value, and also get in the way of doing stochastic simulations where we actually want to sample many different futures from a given start point.

The only time where a user actually wants to restore the random number generator state is probably when writing tests that compare a continuous run against a resumed one. They can opt-in to that behaviour with a new argument to simulation_loop called restore_random_state.

Copy link
Member

@giovannic giovannic left a comment

Choose a reason for hiding this comment

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

Great!

@plietar plietar changed the base branch from checkpoint3 to dev January 29, 2024 13:41
It can have suprising side effects by resetting the user global RNG
state to a fixed value, and also get in the way of doing stochastic
simulations where we actually want to sample many different futures from
a given start point.

The only time where a user actually wants to restore the random number
generator state is probably when writing tests that compare a continuous
run against a resumed one. They can opt-in to that behaviour with a new
argument to `simulation_loop` called `restore_random_state`.
Copy link

codecov bot commented Jan 29, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (c9cdee3) 96.28% compared to head (2eace4c) 96.25%.
Report is 4 commits behind head on dev.

❗ Current head 2eace4c differs from pull request most recent head a22d7e3. Consider uploading reports for the commit a22d7e3 to get more accurate results

Files Patch % Lines
src/event.cpp 92.50% 3 Missing ⚠️
R/simulation.R 96.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #181      +/-   ##
==========================================
- Coverage   96.28%   96.25%   -0.03%     
==========================================
  Files          36       36              
  Lines        1722     1817      +95     
==========================================
+ Hits         1658     1749      +91     
- Misses         64       68       +4     

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

@plietar plietar merged commit 4fb9127 into dev Jan 29, 2024
5 checks passed
@plietar plietar deleted the checkpoint4 branch February 13, 2024 13:30
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.

2 participants