Add global random number generator; support in shooting pt selectors and pathmovers #998

sroet · 2021-04-06T13:07:34Z

This week my CI failed with:

=================================== FAILURES ===================================
______________________ TestGaussianBiasSelector.test_pick ______________________

self = <openpathsampling.tests.test_shooting.TestGaussianBiasSelector object at 0x7fac57d4fc10>

    def test_pick(self):
        picks = [self.sel.pick(self.mytraj) for _ in range(100)]
        pick_counter = collections.Counter(picks)
        assert set(pick_counter.keys()) == set(range(len(self.mytraj)))
        # final test: 99.5 should happen more than 32.4
>       assert pick_counter[2] > pick_counter[0]
E       assert 13 > 18

openpathsampling/tests/test_shooting.py:70: AssertionError

This PR attaches a random number generator to the selectors that can be seeded during tests to prevent these flaky tests.

The current implementation requires numpy>=1.17 (that is when numpy swapped from RandomState to Generator) @dwhswenson please let me know if that is too strict of a requirement and if I should write some code to support the Legacy RandomState

dwhswenson · 2021-04-06T13:49:24Z

The current implementation requires numpy>=1.17 (that is when numpy swapped from RandomState to Generator) @dwhswenson please let me know if that is too strict of a requirement and if I should write some code to support the Legacy RandomState

This is fine in principle: my view is that we should follow NEP 29 with the exception of our until-2.0 support for Python 2.7 (and that the NEP 29 rule for NumPy should also apply to other dependencies.) NumPy 1.16 support could therefore be dropped as of 13 Jan 2021; see drop table in that link. However, it looks like NumPy 1.17 was not released for Python 2.7, so we need an approach compatible with Python 2.7.

That said, fixing the test is probably better than pinning the seed. That test takes a trajectory with 5 possible shooting points, and considers two of them: frame A has about 1/3 the selection probability of frame B. The test assumes that after 100 picks, frame B will have been picked more frequently than frame A. However, apparently the odds of that going the other way is enough that we've noticed multiple failures. Maybe the better approach is to increase the number of picks, or enhance the gap in probability. This test is supposed to be an integration test; if the random number generator changed in some way that it was no longer using the probabilities we provide, we would want the test to be flaky -- that would be a sign of a problem.

BTW, I'm +1 on moving random number generators to instance variables, although I think I'd tend to prefer defaulting to a global RNG (to simplify seeding for a whole simulation in the future), rather than a separate RNG instance for each OPS object instance. Maybe create the RNG instance as paths.default_rng, and then it can be set as the default in the instances.

sroet · 2021-04-06T14:16:18Z

That said, fixing the test is probably better than pinning the seed. That test takes a trajectory with 5 possible shooting points, and considers two of them: frame A has about 1/3 the selection probability of frame B. The test assumes that after 100 picks, frame B will have been picked more frequently than frame A. However, apparently the odds of that going the other way is enough that we've noticed multiple failures. Maybe the better approach is to increase the number of picks, or enhance the gap in probability. This test is supposed to be an integration test; if the random number generator changed in some way that it was no longer using the probabilities we provide, we would want the test to be flaky -- that would be a sign of a problem.

That seems fair, I increased the number of picks to 1000 in #999 .

BTW, I'm +1 on moving random number generators to instance variables, although I think I'd tend to prefer defaulting to a global RNG (to simplify seeding for a whole simulation in the future), rather than a separate RNG instance for each OPS object instance. Maybe create the RNG instance as paths.default_rng, and then it can be set as the default in the instances.

I will rewrite this PR with a single rng instance and add some compatability code for python 2.7 as well

dwhswenson · 2021-04-07T11:02:09Z

I updated the title to better reflect what I understand this will contain; better for release notes. Please correct if the current title doesn't represent the intent of the PR.

sroet · 2021-04-07T11:49:00Z

openpathsampling/pathmover.py

    def __init__(self):
        StorableNamedObject.__init__(self)
-
+        self._rng = default_rng()


This has to be underscored for the safe/load cycle to work

sroet · 2021-04-07T11:50:07Z

openpathsampling/random.py

@@ -0,0 +1,24 @@
+import numpy as np
+import random


added the random import here to (hopefully) make the import less confusing for python2.7

codecov · 2021-04-07T15:54:14Z

Codecov Report

Merging #998 (b16925e) into master (5f86032) will increase coverage by 0.58%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #998      +/-   ##
==========================================
+ Coverage   80.62%   81.20%   +0.58%     
==========================================
  Files         138      139       +1     
  Lines       14695    15130     +435     
==========================================
+ Hits        11848    12287     +439     
+ Misses       2847     2843       -4

Impacted Files	Coverage Δ
openpathsampling/__init__.py	`100.00% <100.00%> (ø)`
openpathsampling/pathmover.py	`84.89% <100.00%> (+1.03%)`	⬆️
openpathsampling/pathmovers/spring_shooting.py	`100.00% <100.00%> (ø)`
openpathsampling/random.py	`100.00% <100.00%> (ø)`
openpathsampling/sample.py	`75.35% <100.00%> (+0.64%)`	⬆️
openpathsampling/shooting.py	`100.00% <100.00%> (ø)`
openpathsampling/deprecations.py	`100.00% <0.00%> (ø)`
openpathsampling/analysis/tis/core.py	`100.00% <0.00%> (ø)`
openpathsampling/analysis/tis/flux.py	`100.00% <0.00%> (ø)`
openpathsampling/snapshot_modifier.py	`100.00% <0.00%> (ø)`
... and 74 more

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 5f86032...b16925e. Read the comment docs.

sroet · 2021-04-07T15:57:14Z

openpathsampling/random.py

@@ -0,0 +1,25 @@
+from __future__ import absolute_import


This is needed as python 2.7 does not understand that I don't want to import this file whenever I say import random in this file...

without this line:

Python 2.7.15 | packaged by conda-forge | (default, Mar 5 2020, 14:56:06) [GCC 7.3.0] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> from openpathsampling.random import random >>> random.random <module 'openpathsampling.random' from 'openpathsampling/random.py'> >>> random.random.random <module 'openpathsampling.random' from 'openpathsampling/random.py'> >>> random.random.random.random <module 'openpathsampling.random' from 'openpathsampling/random.py'>

with this line:

Python 2.7.15 | packaged by conda-forge | (default, Mar 5 2020, 14:56:06) [GCC 7.3.0] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> from openpathsampling.random import random >>> random.random <built-in method random of Random object at 0x555a53ee5080>

sroet · 2021-04-07T16:07:30Z

@dwhswenson Finally figured out what was wrong with python 2.7 (I don't even understand how that import could ever be resolved in the way it did...).

While hunting this down I also added the rng to PathMovers.
This should now be ready for a review!

dwhswenson

Most changes very minor. One point I'm a little confused about in the implications of making default_rng a function, instead of just importing an instance. That's worth some additional discussion.

openpathsampling/random.py

dwhswenson · 2021-04-07T17:03:04Z

openpathsampling/sample.py

@@ -1,4 +1,4 @@
-import random
+from openpathsampling.random import random


I take it this is the necessary way of getting to stdlib random? (Long term -- being before 2.0 -- everything in OPS should be using paths.default_rng)

yes, just to make name-space clashes less likely

openpathsampling/tests/test_pathmover.py

dwhswenson · 2021-04-07T17:10:55Z

openpathsampling/random.py

+    rng = np.random.default_rng()
+
+
+def default_rng():


I don't think this should be a function. Is there a specific reason it must be? Here are my reasons:

A function implies to users that this is creating something.

The behavior is specifically different from what np.random.default_rng() does.

If you do your TestRandom.test_identities test with np.random.default_rng(), it will fail the is test.

Instead, either name rng as default_rng or DEFAULT_RNG (to really emphasize that it is constant). You can still import it into the main openpathsampling namespace.

nvm. I think this has to be a function so that the underlying rng object can change, but newly created instances get the new rng. Nonetheless, please rename rng to something like DEFAULT_RNG.

If I understand correctly, in that case, setting the seed before a simulation starts would be something like paths.random.DEFAULT_RNG = np.random.default_rng(seed) before creating any objects. Is that right?

Nonetheless, please rename rng to something like DEFAULT_RNG

Will do

If I understand correctly, in that case, setting the seed before a simulation starts would be something like paths.random.DEFAULT_RNG = np.random.default_rng(seed) before creating any objects. Is that right?

Yes, exactly (except that it has to go through paths.random.RandomState(seed) for np<1.17) .

Co-authored-by: David W.H. Swenson <dwhs@hyperblazer.net>

dwhswenson · 2021-04-08T14:20:00Z

LGTM!

allow for rng seeding in selectors

45c32eb

sroet mentioned this pull request Apr 6, 2021

make test less flaky #999

Merged

sroet added 3 commits April 6, 2021 16:18

revert change to test

b5cec2c

add default_rng to openpathsampling

3a2b20d

pep8 on test_random.py

fc25258

dwhswenson added the enhancement label Apr 7, 2021

dwhswenson changed the title ~~allow for rng seeding in selectors~~ Add global random number generator; support in shooting pt selectors Apr 7, 2021

Swap over pathmovers as well

ca57009

sroet changed the title ~~Add global random number generator; support in shooting pt selectors~~ Add global random number generator; support in shooting pt selectors and pathmovers Apr 7, 2021

sroet commented Apr 7, 2021

View reviewed changes

sroet added 3 commits April 7, 2021 14:50

force absolute import for py27

32b9a34

finaly got a py27 env working, moved future to the right place

c8c3679

also add the file where future was removed

f5b6277

sroet commented Apr 7, 2021

View reviewed changes

dwhswenson requested changes Apr 7, 2021

View reviewed changes

sroet and others added 2 commits April 8, 2021 10:38

Apply suggestions from code review

24cdbd0

Co-authored-by: David W.H. Swenson <dwhs@hyperblazer.net>

handle comments from @dwhswenson

00d1ffd

sroet requested a review from dwhswenson April 8, 2021 08:43

remove __init__

b16925e

dwhswenson approved these changes Apr 8, 2021

View reviewed changes

dwhswenson merged commit 01041df into openpathsampling:master Apr 8, 2021

sroet deleted the add_generator_to_selection branch April 8, 2021 14:22

dwhswenson mentioned this pull request Apr 10, 2021

Problem with selector RNGs and serialization #1007

Closed

dwhswenson mentioned this pull request Apr 20, 2021

rename random => rng #1015

Merged

dwhswenson mentioned this pull request Jul 5, 2021

OpenPathSampling 1.5 #1031

Merged

sroet mentioned this pull request Aug 27, 2021

Fix documentation issues #1057

Merged

		@@ -1,4 +1,4 @@
		import random
		from openpathsampling.random import random

Add global random number generator; support in shooting pt selectors and pathmovers #998

Add global random number generator; support in shooting pt selectors and pathmovers #998

Uh oh!

Conversation

sroet commented Apr 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dwhswenson commented Apr 6, 2021

Uh oh!

sroet commented Apr 6, 2021

Uh oh!

dwhswenson commented Apr 7, 2021

Uh oh!

sroet Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

sroet Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sroet Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sroet commented Apr 7, 2021

Uh oh!

dwhswenson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dwhswenson Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

sroet Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dwhswenson Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

dwhswenson Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

sroet Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

dwhswenson commented Apr 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sroet commented Apr 6, 2021 •

edited

Loading

sroet Apr 7, 2021 •

edited

Loading

codecov bot commented Apr 7, 2021 •

edited

Loading

sroet Apr 7, 2021 •

edited

Loading