Pass explicit RNG object to methods involving stochasticity #95

tsj5 · 2022-01-22T20:05:51Z

This is a rebase and squash of the feature branch for PR #81 and contains no new commits. The PR message for #81 is reproduced below.

The motivation for this PR comes from CliMA/CalibrateEmulateSample.jl#121; see discussion there.

In order to have reproducible tests involving randomness, it's not sufficient to set the global seed for the default RNG: the default RNG algorithm used by Julia was changed in version 1.7, and the docs for the Random package note that the same seed may give different random values in different versions. For this reason, recommended practice for reproducible tests is to pass an RNG object explicitly to all methods which need it. Julia convention is that the RNG is an optional first positional argument in functions which require it (see e.g. Random, StatsBase, Turning.jl...), and this is done here.

This PR implements that change; in brief

An rng field is added to EnsembleKalmanProcesses (i.e. independent of Process, for all implementations).
An optional rng kwarg is added to construct_initial_ensemble(), used in lieu of rng_seed if provided.
An additional method for construct_initial_ensemble() takes the rng as the first argument, causing kwargs to be ignored.
All methods for sample_distribution() now accept an rng as an optional first argument.

In all of the cases above, Random.GLOBAL_RNG is used by default if an RNG is not given: this PR is fully backwards compatible with main. In particular, tests are updated to use an explicit rng, but examples have been left unchanged. It may be desirable to update them as well.

ilopezgp

Great addition @tsj5 !!

src/EnsembleKalmanProcess.jl

test/EnsembleKalmanProcessModule/runtests.jl

tsj5 · 2022-01-22T21:08:48Z

Great addition @tsj5 !!

Thanks for the detailed review! All changes suggested above have been made in c1b164d (assuming you wanted to keep things squished).

tsj5 · 2022-01-22T21:58:23Z

test/EnsembleKalmanProcess/runtests.jl

@@ -158,7 +165,7 @@ const EKP = EnsembleKalmanProcesses
        eki_final_result = vec(mean(get_u_final(ekiobj), dims = 2))

        # kind of arbitrary tolerance here,
-        @test norm(u_star - eki_final_result) < 0.1
+        @test norm(u_star - eki_final_result) < 0.15


This test is the reason CI failed on Ubuntu in previous commit; missed this because I don't have an Ubuntu box to test on.

Based on the comment, I'm increasing the tolerance so that the test passes. For the record, in the earlier version of these tests (EnsembleKalmanProcessModule/runtests.jl) this test was commented out, and the tolerance was set to 0.5.

Mmmmh this seems to still be a problem sometimes. Honestly, this was a bad test to begin with (our bad). Could you instead compare
@test norm(u_star - eki_final_result) < norm(u_star - eki_init_result)?

where
eki_init_result = vec(mean(get_u_prior(ekiobj), dims = 2))

Thanks, that's a better idea than just bumping the threshold!

I'll continue squashing commits onto this branch and copying/cherry-picking them onto tsj/add-explicit-rng #81, to have a paper trail -- let me know if you want me to proceed differently. Apologies for making this messier than it needed to be: I understand that you want to have the PR reviewed and finalized before the squash... 🙁

Done in b55846d

ilopezgp

LGTM

ilopezgp · 2022-01-22T22:06:46Z

bors r+

95: Pass explicit RNG object to methods involving stochasticity r=ilopezgp a=tsj5 This is a rebase and squash of the feature branch for PR #81 and contains no new commits. The PR message for #81 is reproduced below. <hr> The motivation for this PR comes from CliMA/CalibrateEmulateSample.jl#121; see discussion there. In order to have reproducible tests involving randomness, it's not sufficient to set the global seed for the default RNG: the default RNG algorithm used by Julia was [changed](https://julialang.org/blog/2021/11/julia-1.7-highlights/#new_rng_reproducible_rng_in_tasks) in version 1.7, and the [docs](https://docs.julialang.org/en/v1/stdlib/Random/#Reproducibility) for the Random package note that the same seed may give different random values in different versions. For this reason, [recommended practice](https://discourse.julialang.org/t/set-the-state-of-the-global-rng/12599/3) for reproducible tests is to pass an RNG object explicitly to all methods which need it. Julia convention is that the RNG is an optional first positional argument in functions which require it (see e.g. Random, StatsBase, Turning.jl...), and this is done here. This PR implements that change; in brief - An `rng` field is added to `EnsembleKalmanProcesses` (i.e. independent of `Process`, for all implementations). - An optional `rng` kwarg is added to `construct_initial_ensemble()`, used in lieu of `rng_seed` if provided. - An additional method for `construct_initial_ensemble()` takes the rng as the first argument, causing kwargs to be ignored. - All methods for `sample_distribution()` now accept an rng as an optional first argument. In all of the cases above, `Random.GLOBAL_RNG` is used by default if an RNG is not given: this PR is fully backwards compatible with `main`. In particular, tests are updated to use an explicit rng, but examples have been left unchanged. It may be desirable to update them as well. Co-authored-by: Thomas Jackson <[email protected]>

bors · 2022-01-22T22:11:38Z

Build failed:

test_success

Pass explicit `rng` object to all methods using randomness, in order to have better reproducibility. This is recommended best practice in https://discourse.julialang.org/t/set-the-state-of-the-global-rng/12599/3 Changes are fully backwards-compatible, as verified by exisiting unit tests. Add `rng` member to EnsembleKalmanProcess. Add it as a kwarg to outer constructors, with default value Random.GLOBAL_RNG. Use `rng` in update_ensemble! method. Add `rng` to the end of (required) positional arguments for ParameterDistribution.sample_distribution. Add `rng` as an optional recognized kwarg for construct_initial_ensemble. Add unit tests for sample_distribution Specify RNG algorithm in unit tests In all unit tests involving randomness, create an `rng` object and explicitly pass it to all samplers. This will simplify maintaning tests when Julia's RNG implementation changes, as it did in 1.6 -> 1.7. Add rng argument description to docstrings Use StableRNG to test 2nd rng algorithm The Random module in julia < 1.7 only implements one RNG, MersenneTwister (there's RandomDevice, but that's not seedable/reproducible). To write unit tests for RNG dependence, we need tests with a second algorithm. We use StableRNGs.jl for this purpose (only listed as a dependency for test). Make rng optional 1st positional arg for sample_distribution() This is done to agree with widespread julia convention: see Random, StatsBase, Turning.jl, ... Define additional methods that use the default value, Random.GLOBAL_RNG, if an rng isn't given as the first argument. Allow rng as optional 1st arg for construct_initial_ensemble() This is done to agree with widespread julia convention: see Random, StatsBase, Turning.jl, ... Add sample_distribution() methods for optional args Test coverage for all methods of sample_distribution() Run climaformat.jl EnsembleKalmanProcessModule tests renamed in main Fix seed vs. rng; standardize refs to AbstractRNG Run climaformat.jl Increase (arbitrary) tolerance for failing EKI test Fix use of rand() without explicit rng Absolute -> relative EKP convergence test

tsj5 · 2022-01-22T23:19:54Z

I'm a bit confused by CI results for 9d59c94: the problematic EKP unit test passes for julia-1.7/ubuntu but fails for julia-latest/ubuntu, even though those test envs use the same version of julia and ubuntu.

The unit test has been re-written to be more robust, as suggested here, but the fact that this problem is appearing in this way is troubling, since the entire purpose of this PR is to make the package more reproducible.

I found some places where the rng wasn't being passed through, and fixed them in e4bb0d4 (squashed here in b55846d). The changes aren't in the code path for the failing test, though. I'll keep looking into this non-reproducibility as best as I can, since I don't have an ubuntu image handy.

ilopezgp · 2022-01-22T23:22:58Z

I'm a bit confused by CI results on the previous commit: the problematic EKP unit test passes for julia-1.7/ubuntu but fails for julia-latest/ubuntu, even though those test envs use the same version of julia and ubuntu. This is troubling, since the entire purpose of this PR is to make the package more reproducible.

I found some places where the rng wasn't being passed through, and fixed them in e4bb0d4 (squashed here in b55846d). The changes aren't in the code path for the failing test, though. I'll keep looking into this non-reproducibility as best as I can, since I don't have an ubuntu image handy.

That's a good point! It seems one of the random seeds is changing between architectures, Julia versions, or maybe just instances of the test.

ilopezgp · 2022-01-22T23:41:15Z

bors r+

bors · 2022-01-22T23:49:11Z

Build succeeded:

ilopezgp · 2022-01-23T00:02:36Z

Closes #55

tsj5 force-pushed the tsj/add-explicit-rng-rebase branch from a6e7d2e to 84d422d Compare January 22, 2022 20:26

ilopezgp self-requested a review January 22, 2022 20:27

tsj5 mentioned this pull request Jan 22, 2022

Pass explicit RNG object to methods involving stochasticity #81

Closed

ilopezgp reviewed Jan 22, 2022

View reviewed changes

src/EnsembleKalmanProcess.jl Outdated Show resolved Hide resolved

src/EnsembleKalmanProcess.jl Outdated Show resolved Hide resolved

src/EnsembleKalmanProcess.jl Show resolved Hide resolved

test/EnsembleKalmanProcessModule/runtests.jl Outdated Show resolved Hide resolved

tsj5 force-pushed the tsj/add-explicit-rng-rebase branch from 84d422d to c1b164d Compare January 22, 2022 21:01

tsj5 force-pushed the tsj/add-explicit-rng-rebase branch 2 times, most recently from 7f0e855 to 8953876 Compare January 22, 2022 21:52

tsj5 commented Jan 22, 2022

View reviewed changes

ilopezgp approved these changes Jan 22, 2022

View reviewed changes

tsj5 force-pushed the tsj/add-explicit-rng-rebase branch from 8953876 to b55846d Compare January 22, 2022 23:03

tsj5 force-pushed the tsj/add-explicit-rng-rebase branch from b55846d to 31316cf Compare January 22, 2022 23:17

bors bot merged commit c2c567a into CliMA:main Jan 22, 2022

ilopezgp linked an issue Jan 23, 2022 that may be closed by this pull request

Random seeds #55

Closed

ilopezgp mentioned this pull request Jan 23, 2022

Random seeds #55

Closed

tsj5 mentioned this pull request Jan 24, 2022

Use EKP.jl 0.2.0 & pass explicit RNG object where used CliMA/CalibrateEmulateSample.jl#122

Merged

ilopezgp mentioned this pull request Feb 4, 2022

Deterministic behavior of stochastic functions should not be the default #104

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass explicit RNG object to methods involving stochasticity #95

Pass explicit RNG object to methods involving stochasticity #95

tsj5 commented Jan 22, 2022 •

edited

Loading

ilopezgp left a comment

tsj5 commented Jan 22, 2022

tsj5 Jan 22, 2022

ilopezgp Jan 22, 2022 •

edited

Loading

tsj5 Jan 22, 2022

tsj5 Jan 22, 2022

ilopezgp left a comment

ilopezgp commented Jan 22, 2022

bors bot commented Jan 22, 2022

tsj5 commented Jan 22, 2022 •

edited

Loading

ilopezgp commented Jan 22, 2022 •

edited

Loading

ilopezgp commented Jan 22, 2022

bors bot commented Jan 22, 2022

ilopezgp commented Jan 23, 2022

Pass explicit RNG object to methods involving stochasticity #95

Pass explicit RNG object to methods involving stochasticity #95

Conversation

tsj5 commented Jan 22, 2022 • edited Loading

ilopezgp left a comment

Choose a reason for hiding this comment

tsj5 commented Jan 22, 2022

tsj5 Jan 22, 2022

Choose a reason for hiding this comment

ilopezgp Jan 22, 2022 • edited Loading

Choose a reason for hiding this comment

tsj5 Jan 22, 2022

Choose a reason for hiding this comment

tsj5 Jan 22, 2022

Choose a reason for hiding this comment

ilopezgp left a comment

Choose a reason for hiding this comment

ilopezgp commented Jan 22, 2022

bors bot commented Jan 22, 2022

tsj5 commented Jan 22, 2022 • edited Loading

ilopezgp commented Jan 22, 2022 • edited Loading

ilopezgp commented Jan 22, 2022

bors bot commented Jan 22, 2022

ilopezgp commented Jan 23, 2022

tsj5 commented Jan 22, 2022 •

edited

Loading

ilopezgp Jan 22, 2022 •

edited

Loading

tsj5 commented Jan 22, 2022 •

edited

Loading

ilopezgp commented Jan 22, 2022 •

edited

Loading