MAINT/ENH: refactor np.random.x to use np.random.Generator by tupui · Pull Request #5357 · scikit-image/scikit-image

tupui · 2021-04-26T17:59:38Z

Description

Remove usage of np.random.RandomState in code and examples in favour of np.random.Generator. Only kept in tests as this is still the recommended way.
Also this change all direct calls to np.random.x to use a np.random.Generator (also recommended).

For reviewers

Check that the PR title is short, concise, and will make sense 1 year
later.
Check that new functions are imported in corresponding __init__.py.
Check that new features, API changes, and deprecations are mentioned in
doc/release/release_dev.rst.

pep8speaks · 2021-04-26T17:59:50Z

Hello @tupui! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file skimage/data/_registry.py:

Line 141:80: E501 line too long (107 > 79 characters)
Line 146:80: E501 line too long (109 > 79 characters)
Line 147:80: E501 line too long (110 > 79 characters)

Comment last updated at 2021-06-04 18:14:35 UTC

tupui · 2021-04-26T18:02:44Z

@jni I started to work on this. I did a first pass at everything. I pushed so that this can get feedbacks and also to see the CI.

grlee77 · 2021-04-26T19:05:09Z

I see that in a couple of places a new argument was added:
1.) added seed kwarg to medial_axis
2.) added random_seed kwarg to unsupervised_wiener
I think these should probably be keyword-only arguments (i.e. ,*, seed=None)

it seems we were not very consistent in naming the argument seed vs. random_seed across the library (I think I also saw sample_seed in one place). Making this more consistent is something to consider separately for the 1.0 API.

tupui · 2021-04-26T19:40:51Z

I see that in a couple of places a new argument was added:

1.) added seed kwarg to medial_axis

2.) added random_seed kwarg to unsupervised_wiener

I think these should probably be keyword-only arguments (i.e. ,*, seed=None)

it seems we were not very consistent in naming the argument seed vs. random_seed across the library (I think I also saw sample_seed in one place). Making this more consistent is something to consider separately for the 1.0 API.

Oh yes sorry i forgot to mention that. I can make them keyword only. Let me know the name of the keyword I should use.

jni · 2021-04-27T08:10:51Z

@tupui Gah, looks like something went wrong with a rebase...?

tupui · 2021-04-27T08:14:08Z

Yes, will fix it. I had master which was in front of the changes I wanted to get from the NumPy bump PR.

jni

Thanks @tupui! I've left some minor comments but generally 👍 on the PR. Should be easy to get this over the line. 🙏

jni · 2021-04-27T07:42:39Z

-noise = np.random.random(noisy_image.shape)
+rng = np.random.default_rng()
+noise = rng.random(noisy_image.shape)


This is going to take a long time to undo in my brain... 😅

Haha totally, I also still need to check NumPy's doc from time to time because I am using the wrong methods.

jni · 2021-04-27T08:37:19Z

-    >>> np.random.seed(seed=1234)
-    >>> data += np.random.normal(size=data.shape)
+    >>> rng = np.random.default_rng()
+    >>> data += rng.normal(size=data.shape)


Can we perhaps continue setting the seed here (and below) so that the doctest can still be run? In some cases it's unclear whether things are suitably tested without the doctest...!

Do you have a CI job for the doctests?

We had some discussions for SciPy (and still are having some on Astropy) and we came up with a monkeypatching of the line >>> rng = np.random.default_rng() to include a seed. To me this is no practical but it works. Here it does not seems that you have much examples which would really need a seed. Hence I would advice not to use any. And if there are things that are tested in the examples which are not in the tests, then we should simply move these to tests. In the end it's double the work for you maintainers to maintain 2 test suites otherwise.

Yes, the doctests are configured run by default. imho the double effort is in the other direction — if we add tests in addition to doctests (which no longer become doctests), then we have to maintain them in two places.

I was more thinking about moving than duplicating. IMO doctests should only serve to check that examples are still aligned with the implementation.

Anyway, what I can propose is to either put a seed when really necessary and comment with a DO NOT COPY. So that you could still use them for testing. For the seed itself, I would not use the same for all examples and I would get it using np.random.SeedSequence. Would that be ok?

Yup, that all sounds fine! Sometimes an example usage is close enough to what a test would be that you can kill two birds with one stone — and we do this liberally at skimage. 😊 😬

@hmaarrfk we had this discussion. So in the example you noted, I missed to do this. Let me know what you choose so I modify it accordingly if needed.

Co-authored-by: Juan Nunez-Iglesias <[email protected]>

tupui · 2021-04-29T19:02:21Z

I fixed the tests. I just have the following issues. For the first 2, I guess I need to update the files? For the last 2 I am not sure why this crashes.

Can you help with that @jni?

I will have a look at the examples now.

_______________________________ test_3d_vs_fiji ________________________________

        img_f = io.imread(fetch("data/_blobs_3d_fiji_skeleton.tif"))

___________________________ test_unsupervised_wiener ___________________________
    
        path = fetch('restoration/tests/camera_unsup.npy')

________________________________ test_localvar _________________________________

passing but no stable with different seeds

_________________________________ test_speckle _________________________________

scikit-image/skimage/util/tests/test_random_noise.py:143: AssertionError

_________________________________ test_poisson _________________________________

scikit-image/skimage/util/tests/test_random_noise.py:154: AssertionError

this is necessary due to change in the random noise from the new generator

grlee77 · 2021-04-30T18:08:56Z

Can you help with that @jni?

I fixed the test_speckle and test_poisson and regenerated the data for test_unsupervised_wiener and test_3d_vs_fiji. ~~For test_3d_vs_fiji, I assume the original reference data was generated using Fiji? I didn't try updating that one here.~~

grlee77 · 2021-04-30T20:16:27Z

I think the only failing tests now are the ones related to the change in the sha256 for the 3 files that were updated. In some of the CI logs these are being downloaded from the main branch rather than this one, which is causing the mismatch:

for example:

Downloading file 'data/_blobs_3d_fiji_skeleton.tif' from 'https://github.com/scikit-image/scikit-image/raw/main/skimage/data/_blobs_3d_fiji_skeleton.tif' to '/home/runner/.cache/scikit-image/main'

so it is downloading the old version of the file, but comparing it to the sha256 for the new one

I assume this will just work once we merge this branch.

hmaarrfk · 2021-05-02T15:01:11Z

-    seed : int, optional
-        Seed to initialize the random number generator.
-        If `None`, a random seed from the operating system is used.
+    seed : {None, int, `numpy.random.Generator`}, optional


This is a new feature and should be highlighted. I Think we things like .. versionadded::

Indeed, also not clear what to choose as per #5359. And probably I should add it as keyword argument only with ..., *, seed. What should I put for the version?

Yes, please make any added parameters keyword-only.

Use 0.19 for the versionadded statement.

ok thanks @grlee77. And for the naming?

I don't think we reached any conclusion in #5359 on the preferred name, but I am personally leaning toward random_state. Unless there are objections, we could use that for any newly added parameter here. I wouldn't rename any of the pre-existing parameters in this PR. Let's leave that to a potential follow-up.

To be clear, in this case I did not add the parameter but just the documentation and called behind default_rng instead of RandomState. But there are other places where I added the parameter. Correct me if I am wrong, but I should just add versionadded info to the new places but not here.

grlee77 · 2021-05-03T14:40:24Z

I think the only other task aside from the versionadded and keyword-only stuff mentioned above is adding something about this to doc/release/release_dev.rst. The new keywords can be mentioned under the "New Features" section. We should probably also mention that seeds will not give identical results to prior releases due to the change in the underlying generator used.

To help avoid conflicts you may need to merge master to this branch first since a lot of entries left over from the 0.18 release were recently removed from that file in #5367.

tupui · 2021-05-03T15:22:11Z

@grlee77 I think this is ready on my side. I addressed the points you listed and merged master.

grlee77

Thanks @tupui, it looks good to me.

grlee77 · 2021-05-07T19:31:15Z

@hmaarrfk or @jni, can one of you take a quick look at this one again and merge if it seems okay to you?

The CI failures here look like they are because I updated a few of the test files and pooch is using the version from main instead of the ones from this branch.

mkcor

Thank you, @tupui! Would you mind taking care of the (now) conflicting files? Sorry this has been dragging on...

mkcor · 2021-06-04T11:06:32Z

 New Features
 ------------

+- Added a new keyword only parameter ``random_state`` to


Suggested change

- Added a new keyword only parameter ``random_state`` to

- Add a new keyword-only parameter ``random_state`` to

mkcor · 2021-06-04T11:08:13Z

-        Seed to initialize the random number generator.
-        If `None`, a random seed from the operating system is used.
+    seed : {None, int, `numpy.random.Generator`}, optional
+        If `seed` is None the `numpy.random.Generator` singleton is used.


Suggested change

If `seed` is None the `numpy.random.Generator` singleton is used.

If `seed` is None, the `numpy.random.Generator` singleton is used.

mkcor · 2021-06-04T11:09:43Z

-        Seed to initialize the random number generator.
-        If `None`, a random seed from the operating system is used.
+    random_seed : {None, int, `numpy.random.Generator`}, optional
+        If `random_seed` is None the `numpy.random.Generator` singleton is


Suggested change

If `random_seed` is None the `numpy.random.Generator` singleton is

If `random_seed` is None, the `numpy.random.Generator` singleton is

mkcor · 2021-06-04T11:10:04Z

        around keypoints.
-    sample_seed : int, optional
+    sample_seed : {None, int, `numpy.random.Generator`}, optional
+        If `sample_seed` is None the `numpy.random.Generator` singleton is


Suggested change

If `sample_seed` is None the `numpy.random.Generator` singleton is

If `sample_seed` is None, the `numpy.random.Generator` singleton is

mkcor · 2021-06-04T11:10:41Z

-        If None, the random number generator is the RandomState instance used
-        by `np.random`. The random state is used when generating a set of
+    random_state : {None, int, `numpy.random.Generator`}, optional
+        If `random_state` is None the `numpy.random.Generator` singleton is


Suggested change

If `random_state` is None the `numpy.random.Generator` singleton is

If `random_state` is None, the `numpy.random.Generator` singleton is

mkcor · 2021-06-04T11:16:03Z

 def test_3d_vs_fiji():
    # generate an image with blobs and compate its skeleton to
-    # the skeleton generated by FIJI
+    # the skeleton generated by FIJI (Plugins>Skeleton->Skeletonize)


Suggested change

# the skeleton generated by FIJI (Plugins>Skeleton->Skeletonize)

# the skeleton generated by FIJI (Plugins > Skeleton > Skeletonize)

or "(Plugins>Skeleton>Skeletonize)" or "(Plugins->Skeleton->Skeletonize)"

mkcor · 2021-06-04T11:16:38Z

       True by default. If true, pixel values of the result above 1 or
       under -1 are thresholded for skimage pipeline compatibility.
+    random_state : {None, int, `numpy.random.Generator`}, optional
+        If `random_state` is None the `numpy.random.Generator` singleton is


Suggested change

If `random_state` is None the `numpy.random.Generator` singleton is

If `random_state` is None, the `numpy.random.Generator` singleton is

mkcor · 2021-06-04T11:17:59Z

        Whether to return the full segmentation hierarchy tree and distances.
-    random_seed : int
+    random_seed : {None, int, `numpy.random.Generator`}, optional
+        If `random_seed` is None the `numpy.random.Generator` singleton


Suggested change

If `random_seed` is None the `numpy.random.Generator` singleton

If `random_seed` is None, the `numpy.random.Generator` singleton

mkcor · 2021-06-04T11:18:40Z

        Angles at which to compute the transform, in radians.
-    seed : int, optional
+    seed : {None, int, `numpy.random.Generator`, optional}
+        If `seed` is None the `numpy.random.Generator` singleton is used.


Suggested change

If `seed` is None the `numpy.random.Generator` singleton is used.

If `seed` is None, the `numpy.random.Generator` singleton is used.

mkcor · 2021-06-04T11:19:22Z

-    seed : int, optional
-        If provided, this will set the random seed before generating noise,
+    seed : {None, int, `numpy.random.Generator`}, optional
+        If `seed` is None the `numpy.random.Generator` singleton is


Suggested change

If `seed` is None the `numpy.random.Generator` singleton is

If `seed` is None, the `numpy.random.Generator` singleton is

grlee77 · 2021-06-04T17:30:34Z

The conflicts are related to other float32 PRs of mine that were merged, so I can help resolve them here.

grlee77 · 2021-06-04T18:13:48Z

One issue that came up in resolving conflicts is related to the following lines in unsupervised_wiener:

    rng = np.random.default_rng(random_state)

        _rand1 = rng.standard_normal(data_spectrum.shape)
        _rand1 = _rand1.astype(float_type, copy=False)
        _rand2 = rng.standard_normal(data_spectrum.shape)
        _rand2 = _rand2.astype(float_type, copy=False)
        excursion = np.sqrt(0.5 / precision) * (_rand1 + 1j * _rand2)

Here it should be more efficient to pass a dtype argument to standard_normal.

        _rand1 = rng.standard_normal(data_spectrum.shape, dtype=float_type)
        _rand2 = rng.standard_normal(data_spectrum.shape, dtype=float_type)
        excursion = np.sqrt(0.5 / precision) * (_rand1 + 1j * _rand2)

However, this results in a different stream of random values for float32 vs. float64, which breaks comparison to the existing float64 reference data in test_unsupervised_wiener.

Here I used astype after generating float64 random values so that we could reuse the same reference data, but perhaps we should also store separate reference data for the float32 case instead?

tupui · 2021-06-05T05:05:39Z

Thanks @grlee77 for updating the PR.
Sorry I cannot help myself until end of June. I am away without a computer.

mkcor · 2021-06-05T08:35:51Z

However, this results in a different stream of random values for float32 vs. float64, which breaks comparison to the existing float64 reference data in test_unsupervised_wiener.

@grlee77 ok.

Here I used astype after generating float64 random values so that we could reuse the same reference data, but perhaps we should also store separate reference data for the float32 case instead?

Can we do this (in a subsequent PR) by parametrizing test_unsupervised_wiener with [np.float32, np.float64]?

mkcor · 2021-06-05T08:56:02Z

Can we do this (in a subsequent PR) by parametrizing test_unsupervised_wiener with [np.float32, np.float64]?

Oh, it looks like test_unsupervised_wiener could be parametrized(?) with [np.float16, np.float32, np.float64] from (screenshot of failing "linux-cp3.7-optional-deps" tests):

? Although I'm not seeing this in the PR...

I understand that the first two tests (data/tests/test_data.py::test_download_all_with_pooch and morphology/tests/test_skeletonize_3d.py::test_3d_vs_fiji) fail because of hash mismatches, but what about test_unsupervised_wiener?

grlee77 · 2021-06-07T22:05:14Z

I understand that the first two tests (data/tests/test_data.py::test_download_all_with_pooch and morphology/tests/test_skeletonize_3d.py::test_3d_vs_fiji) fail because of hash mismatches, but what about test_unsupervised_wiener?

This failure is for the same reason (the data files used in the tests had to be updated since the random values generated within unsupervised_wiener are different after this PR).

mkcor · 2021-06-08T12:40:11Z

Ok, thank you, @grlee77 (and @tupui!). Merging now.

tupui · 2021-06-08T14:31:36Z

Thanks everyone for your help! Great community 😃

alexdesiqueira added ⏩ type: Enhancement Improve existing features 🔧 type: Maintenance Refactoring and maintenance of internals labels Apr 26, 2021

grlee77 reviewed Apr 26, 2021

View reviewed changes

Comment thread skimage/util/noise.py Outdated

grlee77 reviewed Apr 26, 2021

View reviewed changes

Comment thread skimage/feature/brief.py Outdated

jni mentioned this pull request Apr 27, 2021

API consistency: seed vs random_seed vs sample_seed #5359

Closed

tupui force-pushed the random_state branch from 22a40d2 to 8db68c6 Compare April 27, 2021 07:54

tupui added 5 commits April 27, 2021 10:30

MAINT: minimal NumPy version from scikit-image#5430

b7d4032

MAINT: remove RandomState in favour of Generator

682977b

MAINT: refactor np.random calls to use objects

ccec7a7

STY: pep8

b59be57

MAINT: positional seed and rename to random_state the new ones

b0df971

tupui force-pushed the random_state branch from 9ebf9a8 to b0df971 Compare April 27, 2021 08:31

jni reviewed Apr 27, 2021

View reviewed changes

tupui and others added 2 commits April 27, 2021 10:51

MAINT: positional random_state in _bernoulli

5892d2e

Co-authored-by: Juan Nunez-Iglesias <[email protected]>

MAINT: revert default and imports

7b8d66d

tupui added 2 commits April 29, 2021 21:21

TST/DOC: fix random values in doctests

f6aec27

TST: fix remaining random

903fe30

tupui force-pushed the random_state branch from 80c74cc to 903fe30 Compare April 29, 2021 19:21

grlee77 added 2 commits April 30, 2021 13:39

TST: update expected result in test_speckle and test_poisson

f8c56aa

TST: update files containing expected result of test_unsupervised_wiener

f57a774

this is necessary due to change in the random noise from the new generator

TST: Update FIJI reference data for skeletonize 3D

310a1b7

hmaarrfk reviewed May 2, 2021

View reviewed changes

tupui added 3 commits May 3, 2021 17:05

MAINT: add versionadded information

9b14899

MAINT: update release notes with seeding

8656dc1

Merge remote-tracking branch 'upstream/main' into random_state

42f8100

grlee77 approved these changes May 3, 2021

View reviewed changes

alexdesiqueira mentioned this pull request May 3, 2021

2021's calendar of community management #5169

Closed

tupui mentioned this pull request May 4, 2021

TST: Do not use random seed in doctest astropy/astropy#11566

Open

fix conflict

e389f2b

grlee77 added this to the 0.19 milestone May 10, 2021

mkcor reviewed Jun 4, 2021

View reviewed changes

mkcor mentioned this pull request Jun 4, 2021

Implement Fisher vectors in scikit-image #5349

Merged

grlee77 added the action: mrg+1 label Jun 4, 2021

fix merge conflicts

758f0cc

mkcor approved these changes Jun 8, 2021

View reviewed changes

mkcor merged commit 4004d01 into scikit-image:main Jun 8, 2021

mkcor mentioned this pull request Jun 23, 2021

single precision support in skimage.registration module #5204

Merged

tupui deleted the random_state branch June 28, 2021 17:33

grlee77 mentioned this pull request Jul 2, 2021

prefer use of new-style NumPy random API in tests #5450

Merged

alessiamarcolini mentioned this pull request Dec 6, 2021

Permit skimage 0.19.0 histolab/histolab#363

Closed

10 tasks done

	- Added a new keyword only parameter ``random_state`` to
	- Add a new keyword-only parameter ``random_state`` to

	If `seed` is None the `numpy.random.Generator` singleton is used.
	If `seed` is None, the `numpy.random.Generator` singleton is used.

	If `random_seed` is None the `numpy.random.Generator` singleton is
	If `random_seed` is None, the `numpy.random.Generator` singleton is

	If `sample_seed` is None the `numpy.random.Generator` singleton is
	If `sample_seed` is None, the `numpy.random.Generator` singleton is

	If `random_state` is None the `numpy.random.Generator` singleton is
	If `random_state` is None, the `numpy.random.Generator` singleton is

	# the skeleton generated by FIJI (Plugins>Skeleton->Skeletonize)
	# the skeleton generated by FIJI (Plugins > Skeleton > Skeletonize)

Conversation

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

Uh oh!

Description

For reviewers

Uh oh!

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

Uh oh!

Comment last updated at 2021-06-04 18:14:35 UTC

Uh oh!

tupui commented Apr 26, 2021

Uh oh!

Uh oh!

grlee77 commented Apr 26, 2021

Uh oh!

Uh oh!

tupui commented Apr 26, 2021

Uh oh!

jni commented Apr 27, 2021

Uh oh!

tupui commented Apr 27, 2021

Uh oh!

jni 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!

Uh oh!

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!

Uh oh!

tupui commented Apr 29, 2021

Uh oh!

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

Uh oh!

Uh oh!

grlee77 commented Apr 30, 2021

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!

grlee77 commented May 3, 2021

Uh oh!

tupui commented May 3, 2021

Uh oh!

grlee77 left a comment

Choose a reason for hiding this comment

Uh oh!

grlee77 commented May 7, 2021

Uh oh!

mkcor 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 26, 2021 •

edited

Loading

pep8speaks commented Apr 26, 2021 •

edited

Loading

grlee77 commented Apr 30, 2021 •

edited

Loading