using `np.random.RandomState(seed)` instead of `np.random.seed(seed)` #4250

vmoens · 2021-08-04T15:33:42Z

closing #4247

This PR makes use of np.RandomState() locally in the tests instead of the previous np.random.seed(), so as to contain numpy's RNG locally to test functions instead of leaking the RNG seed globally.

amend

…o np_randomstate

NicolasHug

Thanks @vmoens , this looks good

This rvs issue is unfortunate, I commented below, let's see what @fmassa thinks

NicolasHug · 2021-08-05T09:08:07Z

test/test_datasets.py

@@ -21,6 +21,8 @@
 import torch.nn.functional as F
 from torchvision import datasets

+random_state_numpy = np.random.RandomState(0)


we should create a local RandomState object in each test function instead of having a global one in each file. Otherwise, tests are still dependent on each other withing a single module.

Also, no strong opinion on that but np_rng would be shorter and still be a descriptive name, so I'd suggest to use that instead

NicolasHug · 2021-08-05T09:23:24Z

torchvision/models/inception.py

@@ -123,7 +125,9 @@ def __init__(
                    import scipy.stats as stats
                    stddev = m.stddev if hasattr(m, 'stddev') else 0.1
                    X = stats.truncnorm(-2, 2, scale=stddev)
-                    values = torch.as_tensor(X.rvs(m.weight.numel()), dtype=m.weight.dtype)
+                    values = torch.as_tensor(
+                        X.rvs(m.weight.numel(), random_state=random_state_numpy),


oh, during our convo I had missed that the rvs was within the model's code, I thought it was just in the test. That's a bigger problem than I thought

@fmassa, googlenet and inception call scipy's rvs methods, which draw from numpy's RNG.

That's fairly unexpected I think. Shouldn't we just be relying on pytorch's RNG? I'm thinking of the following workarounds:

add a new np_random_state parameter to the constructor to control that RNG

rely on torch instead of numpy to draw samples from a truncated normal.

I think the second would make much more sense, although I don't know how easy this will be. WDYT?

As a temporary workaround we could use a pytest fixture that sets numpy's RNG and restores it as e.g. in https://gist.github.com/VictorDarvariu/6cede9c79900c6215b5f848993d283c6, but ugh

That's fairly unexpected I think. Shouldn't we just be relying on pytorch's RNG? I'm thinking of the following workarounds:

We should, but PyTorch didn't implement trunc_normal back when we first implemented this model.
It seems now that it has since been implemented in pytorch/pytorch#32397 , so we should replace it to use PyTorch's implementation. It should be fairly straightforward, but it would be good to check if the PyTorch sampling is much slower than scipy's or not, and to make this change in a separate PR.

@vmoens Would you like a submit a PR to change googlenet and inception to rely on torch.nn.init.trunc_normal_ instead?

I think we can keep this one on hold until then

Sure let me do this:

keep np.random.seed(0) in set_seed for now in this PR

do a new PR where np.random.seed(0) is taken away and we use trunc_normal_ for inception

To keep matters separate, we should aim at removing all numpy seedings in one go, so the first step might unnecessary. We'll need to get rid of the rvs calls first before merging this PR IMO

NicolasHug · 2021-08-05T09:23:30Z

torchvision/models/inception.py

@@ -69,7 +70,8 @@ def __init__(
        aux_logits: bool = True,
        transform_input: bool = False,
        inception_blocks: Optional[List[Callable[..., nn.Module]]] = None,
-        init_weights: Optional[bool] = None
+        init_weights: Optional[bool] = None,
+        random_state_numpy: np.random.RandomState = np.random.RandomState(),


if we ever add a new parameter (I'm not sure we should for now, but we might have to), the default should be None

Hum, we don't pass RNG in any of torchvision functions as of now, so I would rather not do it here, as it would involve a larger discussion.

amend

…o np_randomstate

linter

…o np_randomstate

…mstate

NicolasHug

Thanks @vmoens !! I have some minor comments below, but I'll approve now so you can address and merge once ready

NicolasHug · 2021-08-06T08:38:09Z

test/test_image.py

@@ -273,8 +273,9 @@ def test_write_file_non_ascii():
 ])
 def test_read_1_bit_png(shape):
    with get_tmp_dir() as root:
+        np_rng = np.random.RandomState(0)


as a very minor nit (nitpick = feel free not to address): the declaration of np_rng doesn't need to be within the context manager. Same below

so is the rest of the test :p strictly speaking, only image_path = os.path.join(root, f'test_{shape}.png') needs to be there

Well we do need image_path to be valid, so technically we could only remove the last 2 lines. But yeah, it's a nit anyway :)

NicolasHug · 2021-08-06T08:39:35Z

test/test_models.py

@@ -193,7 +192,7 @@ def _check_fx_compatible(model, inputs):
 # the _test_*_model methods.
 _model_params = {
    'inception_v3': {
-        'input_shape': (1, 3, 299, 299)
+        'input_shape': (1, 3, 299, 299),


I realize this is due to previous changes that were reverted, but in general we try to avoid unrelated changes, as it obfuscates git blame. Same for the removal of import sys above, which on his own is actually relevant (and doesn't hurt git blame), but having lots of those in a single PR makes review more difficult. Would you mind reverting those changes?

I agree i'll revert these!

NicolasHug · 2021-08-06T08:40:21Z

test/test_transforms.py

@@ -200,18 +200,20 @@ class TestToTensor:
    def test_to_tensor(self, channels):
        height, width = 4, 4
        trans = transforms.ToTensor()
+        np_rng = np.random.RandomState(0)


I think we can remove this one, as you're already declaring another RandomState below

my bad yeah i missed that one

NicolasHug · 2021-08-06T08:40:38Z

test/test_transforms.py

@@ -225,22 +227,25 @@ def test_to_tensor(self, channels):
    def test_to_tensor_errors(self):
        height, width = 4, 4
        trans = transforms.ToTensor()
+        np_rng = np.random.RandomState(0)


Let's remove this one too

I think they come from a rather chaotic git revert :) should have checked before pushing though

github-actions · 2021-08-06T10:29:45Z

Hey @vmoens!

You merged this PR, but no labels were added.

…ed(seed)` (#4250) Summary: Co-authored-by: Vincent Moens <vmoens@fb.com> Reviewed By: NicolasHug Differential Revision: D30417196 fbshipit-source-id: f53bc950aea4935c164939cab0e14b266e3dd1cb

using np.random.RandomState(seed) instead of np.random.seed(seed)

193b3b6

vmoens requested a review from NicolasHug August 4, 2021 15:33

facebook-github-bot added the cla signed label Aug 4, 2021

Vincent Moens added 3 commits August 5, 2021 09:57

using np.random.RandomState(seed) instead of np.random.seed(seed)

d5dda53

passing random_state_numpy to inception_v3

6659e82

amend

Merge branch 'np_randomstate' of https://github.com/vmoens/vision int…

31bd6e3

…o np_randomstate

NicolasHug reviewed Aug 5, 2021

View reviewed changes

Vincent Moens added 8 commits August 5, 2021 11:18

reverting inception random_state kwarg

746d612

reverting inception random_state kwarg

65dcd02

amend

Merge branch 'np_randomstate' of https://github.com/vmoens/vision int…

d715cb4

…o np_randomstate

move np_rng locally

6e22a1d

move np_rng locally

47cbd1c

linter

Merge branch 'np_randomstate' of https://github.com/vmoens/vision int…

0aeac4d

…o np_randomstate

Merge commit '7e987bfde253cf457f5f6d4d1c35c560f1711a00' into np_rando…

778dea9

…mstate

removing np random seed from test/common_utils.py

674b4b1

vmoens changed the title ~~using np.random.RandomState(seed) instead of np.random.seed(seed)~~ using np.random.RandomState(seed) instead of np.random.seed(seed) Aug 6, 2021

NicolasHug mentioned this pull request Aug 6, 2021

Use torch instead of scipy for random initialization of inception and googlenet weights #4256

Merged

NicolasHug approved these changes Aug 6, 2021

View reviewed changes

Vincent Moens added 2 commits August 6, 2021 10:28

minor

5b0a71b

Merge branch 'master' into np_randomstate

5511cdd

vmoens merged commit 3fa2055 into pytorch:master Aug 6, 2021

vmoens added code quality module: tests labels Aug 6, 2021

fmassa mentioned this pull request Aug 12, 2021

Use numpy RandomState objects in tests instead of drawing from numpy's global RNG #4247

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

using `np.random.RandomState(seed)` instead of `np.random.seed(seed)` #4250

using `np.random.RandomState(seed)` instead of `np.random.seed(seed)` #4250

vmoens commented Aug 4, 2021 •

edited

Loading

NicolasHug left a comment

NicolasHug Aug 5, 2021

NicolasHug Aug 5, 2021

fmassa Aug 5, 2021 •

edited

Loading

NicolasHug Aug 5, 2021

vmoens Aug 5, 2021 •

edited

Loading

NicolasHug Aug 5, 2021 •

edited

Loading

NicolasHug Aug 5, 2021

fmassa Aug 5, 2021

NicolasHug left a comment

NicolasHug Aug 6, 2021

vmoens Aug 6, 2021

NicolasHug Aug 6, 2021

NicolasHug Aug 6, 2021

vmoens Aug 6, 2021

NicolasHug Aug 6, 2021

vmoens Aug 6, 2021

NicolasHug Aug 6, 2021

vmoens Aug 6, 2021

github-actions bot commented Aug 6, 2021

using np.random.RandomState(seed) instead of np.random.seed(seed) #4250

using np.random.RandomState(seed) instead of np.random.seed(seed) #4250

Conversation

vmoens commented Aug 4, 2021 • edited Loading

NicolasHug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmassa Aug 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmoens Aug 5, 2021 • edited Loading

Choose a reason for hiding this comment

NicolasHug Aug 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolasHug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 6, 2021

using `np.random.RandomState(seed)` instead of `np.random.seed(seed)` #4250

using `np.random.RandomState(seed)` instead of `np.random.seed(seed)` #4250

vmoens commented Aug 4, 2021 •

edited

Loading

fmassa Aug 5, 2021 •

edited

Loading

vmoens Aug 5, 2021 •

edited

Loading

NicolasHug Aug 5, 2021 •

edited

Loading