Skip to content

make rng private for selectors #1008

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 12, 2021
Merged

make rng private for selectors #1008

merged 2 commits into from
Apr 12, 2021

Conversation

@sroet
Copy link
Member

@sroet sroet commented Apr 12, 2021 •
edited
Loading

closes #1007 (partly)

The notebook from that issue now works. However, It is now uncertain is the state of the generators that is loaded.
This depends whether OPS is re-imported between loading or not.

# before simulation:
{'bit_generator': 'PCG64',
 'state': {'state': 336467696170254096309584224979479325155,
  'inc': 238122374084886836437703854582102603205},
 'has_uint32': 0,
 'uinteger': 0}
<numpy.random._pcg64.PCG64 at 0x7f0b819d9d10>
 
# after running:
 {'bit_generator': 'PCG64',
 'state': {'state': 195230481313007235580189205770362297672,
  'inc': 238122374084886836437703854582102603205},
 'has_uint32': 1,
 'uinteger': 2214207804}
 <numpy.random._pcg64.PCG64 at 0x7f0b819d9d10>
 
# loaded in the same kernel session
{'bit_generator': 'PCG64',
 'state': {'state': 195230481313007235580189205770362297672,
  'inc': 238122374084886836437703854582102603205},
 'has_uint32': 1,
 'uinteger': 2214207804}
<numpy.random._pcg64.PCG64 at 0x7f0b819d9d10>
 
# loaded in a restarted kernel session
 {'bit_generator': 'PCG64',
 'state': {'state': 316665816032858718566197330052104521012,
  'inc': 90421417105856406385200004491830534511},
 'has_uint32': 0,
 'uinteger': 0}
 <numpy.random._pcg64.PCG64 at 0x7f0ab8347d10>

This is a broader issue to be solved, as OPS does not have a way of storing mutable objects (such as a rng state)

@sroet
Copy link
Member Author

sroet commented Apr 12, 2021

@dwhswenson this is ready for a review

@codecov
Copy link

codecov bot commented Apr 12, 2021

Codecov Report

Merging #1008 (b64fb79) into master (3c5211d) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1008      +/-   ##
==========================================
- Coverage   81.20%   81.19%   -0.02%     
==========================================
  Files         139      139              
  Lines       15130    15130              
==========================================
- Hits        12287    12285       -2     
- Misses       2843     2845       +2
Impacted Files Coverage Δ
openpathsampling/pathmovers/spring_shooting.py 100.00% <100.00%> (ø)
openpathsampling/shooting.py 100.00% <100.00%> (ø)
openpathsampling/netcdfplus/dictify.py 65.47% <0.00%> (-0.55%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c5211d...b64fb79. Read the comment docs.

dwhswenson
dwhswenson approved these changes Apr 12, 2021
View reviewed changes
Copy link
Member

@dwhswenson dwhswenson 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; thanks!

Regarding loading RNG state: It's not clear to me that a user would expect a reloaded shooting point selector to have the same state. Indeed, the way I recommend using OPS is to save all your objects to a setup.db file, and then run simulations by loading those objects (preserve UUIDs across runs). If you did that, the selector reloaded its RNG state, then "independent" path sampling runs would be identical. So I think the expectation is to have a per-session random seed unless the user explicitly requests otherwise.

@dwhswenson dwhswenson merged commit bc1de29 into openpathsampling:master Apr 12, 2021
@sroet sroet deleted the fix_rng_selectors branch April 12, 2021 11:05
@dwhswenson dwhswenson added the bugfix PRs fixing bugs label Apr 25, 2021
@dwhswenson dwhswenson mentioned this pull request Jul 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dwhswenson dwhswenson dwhswenson approved these changes

Assignees

No one assigned

Labels

bugfix PRs fixing bugs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Problem with selector RNGs and serialization

2 participants

@sroet @dwhswenson