MNT: Replace RandomState with default Generator by pllim · Pull Request #10644 · astropy/astropy

pllim · 2020-08-12T21:51:19Z

Description

This pull request is to address the fact that RandomState is now considered legacy.

However, during local testing, it appears that replacing it with default Generator isn't a simple search-and-replace. That is, the results change enough to cause test failures.

Should we proceed with this or trying something else like our own NumpyRNGContext?

@mhvk , do you have any opinion on this?

Fix #11519

Choices

Update the results. But default_rng is not available in Numpy 1.16 or older. ~~Do we copy it into compat or something, or maybe wait to do this till we can drop support for Numpy 1.16?~~ Not a problem now as we have dropped support for Numpy 1.16.
~~Use something else.~~
~~Keep using RandomState.~~

TODO

Wait for MNT: Drop Numpy 1.16 #10664 to merge. ~~If its milestone changes, the milestone here needs to be updated to match.~~
Rebase to pick up the PR above.
Update test results so CI pass. Remember to check the dev/remote job that is allowed to fail.

After merge

Open issue to stop using random seed for doctest, see MNT: Replace RandomState with default Generator #10644 (review) (see TST: Do not use random seed in doctest #11566)
Open issue to follow up on hardcoding test cases in astropy/timeseries/periodograms/bls/tests/test_bls.py and astropy/modeling/tests/test_models.py and assign to @astrofrog , see MNT: Replace RandomState with default Generator #10644 (review) (see TST: Revert hardcoding in test_bls.py and test_models.py #11567)

mhvk

@pllim - I think this is the way to go. I've only followed the discussion about the random-number generator from a distance, but it is clear that the old design had annoying limitations and that the very large break was considered necessary. The new method names certainly seem a lot clearer!

On your question about older numpy: I'd vote for moving to numpy 1.17 in 4.2 - it also means the __array_function__ override is now default, not optional. And with numpy 1.20 getting close, it would seem we're at about the right schedule anyway.

mhvk · 2020-08-15T15:11:17Z

p.s. The failures are of course rather annoying! For tests, we really shouldn't be using random data (unless consciously to flush out bugs, like with hypothesis), while for documentation I can see the point, but for those we may be best off just not checking the final result.

pllim · 2020-08-17T17:38:18Z

we really shouldn't be using random data

Perhaps not, but fixing that is out of scope, as I have no idea what those devs had in mind when they designed the tests. I'll see about updating the results based on local run after #10664 goes in.

pllim · 2020-08-21T22:04:03Z

I tried to update results for the failed tests, but I got stuck with the failures in the following:

astropy.modeling.tests.test_models.TestFittable1DModels.test_deriv_1D -- This is a heavily parametrized test and it is failing for a subset of the inputs during assert_allclose(new_model_with_deriv.parameters, new_model_no_deriv.parameters).
astropy.timeseries.periodograms.bls.tests.test_bls.test_negative_times -- assert_allclose_blsresults(results1, results2) fails for 'transit_time'.

I am also not sure if some of my changes make sense. The relevant subpackage maintainers should look at them carefully.

pllim · 2020-08-21T22:06:38Z

+    x[rng.integers(0, len(t) // 2, N)] = 1
    edges = bayesian_blocks(t, x, fitness='regular_events', dt=dt)
-    assert_allclose(edges, [0, 5.105, 9.99])
+    assert_allclose(edges, [0, 4.365, 4.995, 9.99])


Extra element! Is this supposed to happen?

Based on other comments by @larrybradley , this is probably okay too.

pllim · 2020-08-21T22:07:15Z


    dx, bins = knuth_bin_width(X, return_bins=True)
-    assert_allclose(len(bins), 59)
+    assert_allclose(len(bins), 58)


Off by one! Is this supposed to happen?

Yes, I think this is fine. Different bins for different data.

pllim · 2020-08-21T22:07:52Z

    counts, bins = histogram(X, bins='blocks')
-    assert_allclose(counts, [10, 61, 29])
-    assert_allclose(bins, [-2.55298982, -1.24381059, 0.46422235, 2.26975462])
+    assert_allclose(counts, [3, 97])


One less element! Is this supposed to happen?

Ok for different data.

pllim · 2020-10-21T14:21:46Z

I need input from subpackage maintainers about the failures and unexpected result changes before I can proceed with this.

larrybradley · 2020-10-29T16:57:51Z


    dx, bins = knuth_bin_width(X, return_bins=True)
-    assert_allclose(len(bins), 59)
+    assert_allclose(len(bins), 58)


Yes, I think this is fine. Different bins for different data.

larrybradley · 2020-10-29T16:58:18Z

    counts, bins = histogram(X, bins='blocks')
-    assert_allclose(counts, [10, 61, 29])
-    assert_allclose(bins, [-2.55298982, -1.24381059, 0.46422235, 2.26975462])
+    assert_allclose(counts, [3, 97])


Ok for different data.

pllim · 2020-10-30T17:30:42Z

 def data():
-    rand = np.random.RandomState(123)
-    t = rand.uniform(0, 10, 500)
+    t = np.array([


If this is too hacky, I welcome better ideas.

Probably should open a follow-up issue about this too... cc @adrn

pllim · 2020-10-30T19:14:22Z

RTD failure is unrelated (h5py related). Remote data job passed.

astrofrog

Looks good to me but I think in a follow up we should really try and get rid of the hard coded arrays. I have some ideas for finding a seed so if someone opens the issue it can be assigned to me.

pllim · 2020-11-04T14:51:34Z

@astrofrog , if you can find the seed soon, maybe this PR can wait. If not, feel free to merge. Thanks.

saimn · 2021-04-12T17:28:08Z

This seems ready but needs a rebase.

tupui

@pllim I am a bit late sorry. We did this in SciPy and the only difference is that we removed all seeding from examples. This is to enforce best practices. Using small seeds is bad for the entropy of the generator and also common values like 0, 42, 12345 end up in every code which leads to bias in results. Seeing should only be used for testing.

Here I am proposing to remove the seed and just allow variation in the results to pass the doctests (in SciPy I can add # random or # may vary, I am not sure if this is standard or not. Maybe this is your # doctest: +FLOAT_CMP?).

I can do a PR on this branch if you want some help. Also, I would suggest to add somewhere in the doc a paragraph about seeding. See what we did for example https://scipy.github.io/devdocs/tutorial/stats.html#random-number-generation.

tupui · 2021-04-13T07:18:57Z

    >>> from astropy.stats import biweight_location
-    >>> rand = np.random.RandomState(12345)
-    >>> biloc = biweight_location(rand.randn(1000))
+    >>> rand = np.random.default_rng(12345)


Suggested change

>>> rand = np.random.default_rng(12345)

>>> rand = np.random.default_rng()

tupui · 2021-04-13T07:19:23Z

+    >>> biloc = biweight_location(rand.standard_normal(1000))
    >>> print(biloc)    # doctest: +FLOAT_CMP
-    -0.0175741540445
+    0.01535330525461019


Suggested change

0.01535330525461019

0.01535330525461019 # random

mhvk · 2021-04-13T14:05:35Z

@tupui - that sounds like a very good idea, but we do not have the may vary and random markers yet (might actually be good to replace our FLOAT_CMP with that), and I think we should definitely do that in a follow-up PR, not here.

tupui · 2021-04-13T14:07:44Z

@tupui - that sounds like a very good idea, but we do not have the may vary and random markers yet (might actually be good to replace our FLOAT_CMP with that), and I think we should definitely do that in a follow-up PR, not here.

Then do you suggest to use FLOAT_CMP here an in another PR to have may vary and random? If yes, I could propose the change for this PR and work on the following PR. Let me know 😃

pllim · 2021-04-13T15:11:25Z

Re: No seed for doctest -- FLOAT_CMP only works for small variations. I have no idea what removing the seed would do to the doctest. If you insist on doctest not being deterministic, might as well do a # doctest: +SKIP?

mhvk · 2021-04-13T15:47:01Z

@tupui - I'm suggesting that for this PR we leave the explicit seed in place and use FLOAT_CMP - we can then consider removing the explicit seeding in a separate PR (which will require not using FLOAT_CMP).

adrn · 2021-04-13T16:17:40Z

@tupui - I agree that it is a good idea to figure out how to remove seeding from user-facing examples, but I do agree with @mhvk here that this PR should be restricted to replacing RandomState. Let's make a new issue to follow up on the broader point?

adrn

Took a quick look at this - seems fine to make these changes after rebasing. But in a new issue we should discuss modifying examples to remove seeds.

One other thing that's not clear to me is whether the default bit generator could ever change in numpy? If it does, all the tests could start failing. So if that is a possibility, I wonder if we want to have a test helper that can produce a random number generator given a seed, but with the PCG64 bit generator hard coded? May not be an issue, but others might know...

adrn · 2021-04-13T16:19:28Z

+        # NOTE: PR 10644 replaced deprecated usage of RandomState but could not
+        #       find a new seed that did not cause test failure, resorted to hardcoding.


Hm - seems like this should maybe become a TODO, with a new issue for @nden?

Yeah, haven't gotten any replies here, so follow-up issue is the way to go.

tupui · 2021-04-13T18:43:18Z

@tupui - I agree that it is a good idea to figure out how to remove seeding from user-facing examples, but I do agree with @mhvk here that this PR should be restricted to replacing RandomState. Let's make a new issue to follow up on the broader point?

Sure makes sense. I can propose a following PR if that's ok then.

One other thing that's not clear to me is whether the default bit generator could ever change in numpy? If it does, all the tests could start failing. So if that is a possibility, I wonder if we want to have a test helper that can produce a random number generator given a seed, but with the PCG64 bit generator hard coded? May not be an issue, but others might know...

We had a similar discussion on SciPy. While the underlying bit generator might change, it's a bit unlikely as people are now using it. Still, from NumPy's documentation they explicitly warn about not wanting to ensure reproducible results over the versions. If you must support a lot of versions RandomState is still there and is ensured to provide the same results. But it might not stay forever in NumPy. On SciPy, we still use RandomState in tests because we need to support NumPy 1.16. Maybe this will change once we switch to 1.17.

Attempt to fix most of the failures.

TST: Undo some of the earlier changes, not needed after hardcoding.

pllim · 2021-04-14T20:15:53Z

I rebased. The one failure is unrelated. Given there are multiple approvals, I added some follow-up action items and will merge when CI finishes. Thanks for the reviews and discussions, everyone!

p.s. FWIW, I don't think people should be using random numbers in tests but addressing that is out of scope of this PR.

mhvk · 2021-04-14T21:00:36Z

Yay! And, yes, I agree that one should not use random numbers in tests in the first place.

tupui · 2021-04-14T21:05:43Z

Plus, in the end if the seed is fixed, it's not really a "random" number generator anymore 😉

pllim · 2021-04-14T21:14:22Z

I can't read minds, but I think the "random" numbers got into the tests in one of two ways:

Someone copy-pasted science code into a unit test.
Attempt to mimic "real" data but want to do it as a one-liner.

I don't think randomness was the intention but rather means to an end.

pllim · 2021-04-14T21:19:12Z

I think RTD is stuck due to concurrency limits. #yolo

pllim · 2021-04-14T21:26:03Z

To everyone who is interested in the outstanding issues, please see #11566 and #11567 . Thanks again!

pllim added Refactoring no-changelog-entry-needed labels Aug 12, 2020

pllim added this to the v4.2 milestone Aug 12, 2020

astropy-bot Bot added io.fits modeling stats timeseries visualization labels Aug 12, 2020

mhvk reviewed Aug 15, 2020

View reviewed changes

pllim mentioned this pull request Aug 17, 2020

MNT: Drop Numpy 1.16 #10664

Merged

pllim force-pushed the no-moar-randomstate branch from 66b09b3 to 4229c81 Compare August 21, 2020 22:00

pllim commented Aug 21, 2020

View reviewed changes

Comment thread astropy/modeling/tests/test_models.py Outdated

pllim commented Aug 21, 2020

View reviewed changes

pllim requested review from astrofrog, bsipocz, larrybradley, nden and saimn August 21, 2020 22:11

pllim modified the milestones: v4.2, v4.3 Oct 22, 2020

larrybradley approved these changes Oct 29, 2020

View reviewed changes

pllim force-pushed the no-moar-randomstate branch from 539bda2 to 903b050 Compare October 30, 2020 14:38

pllim marked this pull request as ready for review October 30, 2020 14:55

pllim commented Oct 30, 2020

View reviewed changes

pllim force-pushed the no-moar-randomstate branch from 501eb97 to 91dc9a1 Compare October 30, 2020 17:38

pllim added the Ready-for-final-review label Oct 30, 2020

nden approved these changes Oct 30, 2020

View reviewed changes

astrofrog approved these changes Nov 4, 2020

View reviewed changes

pllim mentioned this pull request Apr 12, 2021

DOC: remove references to RandomState #11519

Closed

tupui reviewed Apr 13, 2021

View reviewed changes

adrn reviewed Apr 13, 2021

View reviewed changes

pllim added 2 commits April 14, 2021 15:42

MNT: Replace RandomState with default Generator.

1737b05

Attempt to fix most of the failures.

TST: Fixed tests in modeling and timeseries.

ce6589d

TST: Undo some of the earlier changes, not needed after hardcoding.

pllim force-pushed the no-moar-randomstate branch from 91dc9a1 to ce6589d Compare April 14, 2021 19:43

pllim merged commit df3e5bb into astropy:main Apr 14, 2021

pllim deleted the no-moar-randomstate branch April 14, 2021 21:19

This was referenced Apr 14, 2021

TST: Do not use random seed in doctest #11566

Open

TST: Revert hardcoding in test_bls.py and test_models.py #11567

Closed

pllim mentioned this pull request May 3, 2021

Modernize random number generation (i.e., stop using np.random.seed/get_state/set_state) #11687

Open

3 tasks

	>>> rand = np.random.default_rng(12345)
	>>> rand = np.random.default_rng()

		# NOTE: PR 10644 replaced deprecated usage of RandomState but could not
		# find a new seed that did not cause test failure, resorted to hardcoding.

Conversation

pllim commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk commented Aug 15, 2020

Uh oh!

pllim commented Aug 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Aug 21, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Oct 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Oct 30, 2020

Uh oh!

astrofrog left a comment

Choose a reason for hiding this comment

Uh oh!

pllim commented Nov 4, 2020

Uh oh!

saimn commented Apr 12, 2021

Uh oh!

tupui left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

tupui Apr 13, 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!

mhvk commented Apr 13, 2021

Uh oh!

tupui commented Apr 13, 2021

Uh oh!

pllim commented Apr 13, 2021

Uh oh!

mhvk commented Apr 13, 2021

Uh oh!

adrn commented Apr 13, 2021

Uh oh!

adrn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tupui commented Apr 13, 2021

Uh oh!

pllim commented Apr 14, 2021

Uh oh!

mhvk commented Apr 14, 2021

Uh oh!

pllim commented Aug 12, 2020 •

edited

Loading

pllim commented Aug 17, 2020 •

edited

Loading

tupui left a comment •

edited

Loading

tupui Apr 13, 2021 •

edited

Loading