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

use a stable rng from StableRNGs for reproducibility #170

Merged
merged 14 commits into from
Sep 18, 2022

Conversation

t-bltg
Copy link
Contributor

@t-bltg t-bltg commented Sep 2, 2022

Is is currently very hard (impossible ?) to rely on Graphs across different julia versions: this is observed in Plots.jl ci through downstream test of GraphRecipes.jl.

The reason is that the RNG across julia minors is documented to be unreliable in terms of reproducibility.

This makes ci testing via image comparison difficult unless duplicating reference files for different julia versions which is quite cumbersome.

It is advised from julia docs, to use StableRNGs.jl instead.

PR:

  1. use StableRNGs in tests;
  2. fix methods signatures using random related calls, to take a rng and seed;
  3. update ci workflows.

@t-bltg t-bltg mentioned this pull request Sep 2, 2022
@codecov
Copy link

codecov bot commented Sep 17, 2022

Codecov Report

Merging #170 (0028764) into master (a3fb98b) will decrease coverage by 0.07%.
The diff coverage is 93.85%.

@@            Coverage Diff             @@
##           master     #170      +/-   ##
==========================================
- Coverage   97.54%   97.47%   -0.08%     
==========================================
  Files         109      109              
  Lines        6314     6332      +18     
==========================================
+ Hits         6159     6172      +13     
- Misses        155      160       +5     

@simonschoelly
Copy link
Member

Thanks for this PR, I think StableRNG might indeed make sense for testing. But this PR would also add it outside of tests? I think we should avoid that, and only use StableRNG for the tests.

We currently might still have some random function that take only a seed instead of a random generator. These functions should be refactored so that they use rng_from_rng_or_seed as a temporary workaround - we then might get rid of the seed in the next Major release.

@t-bltg t-bltg marked this pull request as draft September 17, 2022 20:15
@t-bltg t-bltg force-pushed the get_rng branch 6 times, most recently from 935063c to 77a4e27 Compare September 17, 2022 21:26
@t-bltg
Copy link
Contributor Author

t-bltg commented Sep 17, 2022

I think we should avoid that, and only use StableRNG for the tests.

I've restored the GLOBAL_RNG and used StableRNGs only in the tests.

These functions should be refactored so that they use rng_from_rng_or_seed as a temporary workaround

I've propagated a rng and seed keyword argument to all functions using rand, randperm or other random methods, and replaced all getRNG(seed) calls with rng = rng_from_rng_or_seed(rng, seed).

This took a lot of time / effort to fix all the tests, but upstream GraphRecipes has now reproducible tests.

@simonschoelly, this PR is now ready for review / comments.

@t-bltg t-bltg marked this pull request as ready for review September 17, 2022 22:04
src/utils.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
@t-bltg
Copy link
Contributor Author

t-bltg commented Sep 18, 2022

I think I have addressed all the comments. Upstream tests ✔.

Copy link
Member

@simonschoelly simonschoelly left a comment

Choose a reason for hiding this comment

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

Great, thanks a lot!

@simonschoelly simonschoelly merged commit 8c52ace into JuliaGraphs:master Sep 18, 2022
@t-bltg t-bltg deleted the get_rng branch September 18, 2022 15:53
@t-bltg
Copy link
Contributor Author

t-bltg commented Sep 18, 2022

Amazing, thanks for your valuable comments (and obviously for your time !) 🎉 .

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