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

New approach to handling randomness in pyMOR #1736

Merged
merged 18 commits into from Oct 24, 2022
Merged

New approach to handling randomness in pyMOR #1736

merged 18 commits into from Oct 24, 2022

Conversation

sdrave
Copy link
Member

@sdrave sdrave commented Aug 30, 2022

This PR implements a new approach to handling a global random state in pyMOR. Most notable changes compared to the previous approach:

  • The newer np.random.Generator class is used instead of np.random.RandomState.
  • seed and random_state arguments are removed from all function signatures. Instead to execute some function with a prescribed random state, a new global random generator can be locally installed by used the RNG object returned by new_rng as a context manager:
    with new_rng(seed):
        the_random_function(...)
  • In concurrent code, the spawn_rng wrapper can be used to create child RNGs to ensure deterministic and uncorrelated behavior. Not calling spawn_rng where needed will be detected and cause warnings to be emitted.

@sdrave
Copy link
Member Author

sdrave commented Aug 30, 2022

@pymor/main, @artpelling this should be roughly what we have discussed at the sprint last week. Please take a look if it makes sense to you. (I did not look at the notebooks/tests yet, and documentation is still missing.)

@sdrave sdrave added the pr:change Change in existing functionality label Aug 30, 2022
src/pymor/algorithms/lradi.py Outdated Show resolved Hide resolved
src/pymor/algorithms/lrradi.py Outdated Show resolved Hide resolved
src/pymor/algorithms/samdp.py Outdated Show resolved Hide resolved
src/pymor/algorithms/samdp.py Outdated Show resolved Hide resolved
src/pymor/algorithms/samdp.py Outdated Show resolved Hide resolved
@sdrave
Copy link
Member Author

sdrave commented Sep 1, 2022

@renefritze, @pmli, I have reworked how the random state is set/initialized: init_rng/set_rng are now replaced by a new_rng function, which returns a pyMOR RNG class, which directly derives from numpy.random.Generator. Besides being directly usable as a random generator, RNG has install/uninstall methods, which sets it as pyMOR's global rng. Further it can be used as a context manager to automatically install/uninstall it. In particular, you can now create a single new RNG object and repeatedly set it inside loops and so on.

@sdrave sdrave marked this pull request as ready for review September 2, 2022 09:18
@sdrave sdrave requested review from a team September 2, 2022 09:27
@renefritze renefritze added this to the 2022.2 milestone Sep 2, 2022
@codecov
Copy link

codecov bot commented Sep 2, 2022

Codecov Report

Merging #1736 (8a34c7f) into main (8229a85) will decrease coverage by 0.00%.
The diff coverage is 92.26%.

Additional details and impacted files
Flag Coverage Δ
github_actions 75.19% <85.08%> (+0.03%) ⬆️
gitlab_ci 86.98% <92.26%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pymordemos/parabolic_mor.py 97.08% <ø> (ø)
src/pymordemos/thermalblock.py 95.23% <ø> (ø)
src/pymor/algorithms/lradi.py 87.50% <50.00%> (-0.53%) ⬇️
src/pymor/algorithms/lrradi.py 87.27% <50.00%> (-0.54%) ⬇️
src/pymor/reductors/neural_network.py 88.72% <75.00%> (+0.21%) ⬆️
src/pymor/algorithms/samdp.py 88.96% <80.00%> (+0.03%) ⬆️
src/pymor/tools/random.py 86.88% <86.44%> (-13.12%) ⬇️
src/pymor/algorithms/eigs.py 94.40% <100.00%> (+0.09%) ⬆️
src/pymor/algorithms/hapod.py 90.10% <100.00%> (+0.05%) ⬆️
src/pymor/basic.py 100.00% <100.00%> (ø)
... and 21 more

Copy link
Member

@artpelling artpelling 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 to me (I might have found some commas). I tested it locally with my randomized LinA stuff and it works like a charm!

src/pymor/tools/random.py Outdated Show resolved Hide resolved
src/pymor/tools/random.py Outdated Show resolved Hide resolved
src/pymortests/algorithms/ei.py Show resolved Hide resolved
src/pymortests/algorithms/symplectic.py Show resolved Hide resolved
@artpelling
Copy link
Member

Closes #1564.

@artpelling artpelling mentioned this pull request Sep 26, 2022
5 tasks
docs/source/substitutions.py Outdated Show resolved Hide resolved
@sdrave sdrave force-pushed the global_random_state branch 2 times, most recently from 5a3bcf6 to d21123f Compare October 18, 2022 14:21
@sdrave
Copy link
Member Author

sdrave commented Oct 18, 2022

Ok, so after rebasing CI segfaults on gitlab. @renefritze, any clue why?

@sdrave
Copy link
Member Author

sdrave commented Oct 19, 2022

Seems like some/all other current PRs are affected as well.

@renefritze
Copy link
Member

Ok, so after rebasing CI segfaults on gitlab. @renefritze, any clue why?

None.

@sdrave
Copy link
Member Author

sdrave commented Oct 22, 2022

This seems to be a related but different issue with codecove, @renefritze?

@renefritze
Copy link
Member

This seems to be a related but different issue with codecove, @renefritze?

Yeah, no idea what's even failing there. Turned both total and patch coverage targets to 0 now.

@sdrave sdrave merged commit 8672aa6 into main Oct 24, 2022
@sdrave sdrave deleted the global_random_state branch October 24, 2022 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:change Change in existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants