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.

Sign up for GitHub

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

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

Merged
merged 14 commits into from
Aug 6, 2021
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
193b3b6
using np.random.RandomState(seed) instead of np.random.seed(seed)
Aug 4, 2021
d5dda53
using np.random.RandomState(seed) instead of np.random.seed(seed)
Aug 4, 2021
6659e82
passing random_state_numpy to inception_v3
Aug 5, 2021
31bd6e3
Merge branch 'np_randomstate' of https://github.com/vmoens/vision int…
Aug 5, 2021
746d612
reverting inception random_state kwarg
Aug 5, 2021
65dcd02
reverting inception random_state kwarg
Aug 5, 2021
d715cb4
Merge branch 'np_randomstate' of https://github.com/vmoens/vision int…
Aug 5, 2021
6e22a1d
move np_rng locally
Aug 5, 2021
47cbd1c
move np_rng locally
Aug 5, 2021
0aeac4d
Merge branch 'np_randomstate' of https://github.com/vmoens/vision int…
Aug 5, 2021
778dea9
Merge commit '7e987bfde253cf457f5f6d4d1c35c560f1711a00' into np_rando…
Aug 6, 2021
674b4b1
removing np random seed from test/common_utils.py
Aug 6, 2021
5b0a71b
minor
Aug 6, 2021
5511cdd
Merge branch 'master' into np_randomstate
Aug 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion test/common_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ def get_tmp_dir(src=None, **kwargs):
def set_rng_seed(seed):
torch.manual_seed(seed)
random.seed(seed)
np.random.seed(seed)


class MapNestedTensorObjectImpl(object):
Expand Down
4 changes: 3 additions & 1 deletion test/test_datasets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import torch.nn.functional as F
from torchvision import datasets

random_state_numpy = np.random.RandomState(0)
Copy link
Member

Choose a reason for hiding this comment

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

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



class STL10TestCase(datasets_utils.ImageDatasetTestCase):
DATASET_CLASS = datasets.STL10
Expand Down Expand Up @@ -445,7 +447,7 @@ def inject_fake_data(self, tmpdir, config):

def _create_batch_file(self, root, name, num_images):
data = datasets_utils.create_image_or_video_tensor((num_images, 32 * 32 * 3))
labels = np.random.randint(0, self._VERSION_CONFIG["num_categories"], size=num_images).tolist()
labels = random_state_numpy.randint(0, self._VERSION_CONFIG["num_categories"], size=num_images).tolist()
self._create_binary_file(root, name, {"data": data, self._VERSION_CONFIG["labels_key"]: labels})

def _create_meta_file(self, root):
Expand Down
6 changes: 4 additions & 2 deletions test/test_image.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
IS_WINDOWS = sys.platform in ('win32', 'cygwin')
PILLOW_VERSION = tuple(int(x) for x in PILLOW_VERSION.split('.'))

random_state_numpy = np.random.RandomState(0)


def _get_safe_image_name(name):
# Used when we need to change the pytest "id" for an "image path" parameter.
Expand Down Expand Up @@ -274,7 +276,7 @@ def test_write_file_non_ascii():
def test_read_1_bit_png(shape):
with get_tmp_dir() as root:
image_path = os.path.join(root, f'test_{shape}.png')
pixels = np.random.rand(*shape) > 0.5
pixels = random_state_numpy.rand(*shape) > 0.5
img = Image.fromarray(pixels)
img.save(image_path)
img1 = read_image(image_path)
Expand All @@ -294,7 +296,7 @@ def test_read_1_bit_png(shape):
def test_read_1_bit_png_consistency(shape, mode):
with get_tmp_dir() as root:
image_path = os.path.join(root, f'test_{shape}.png')
pixels = np.random.rand(*shape) > 0.5
pixels = random_state_numpy.rand(*shape) > 0.5
img = Image.fromarray(pixels)
img.save(image_path)
img1 = read_image(image_path, mode)
Expand Down
8 changes: 6 additions & 2 deletions test/test_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,13 @@
import pytest
import warnings
import traceback
import numpy as np


ACCEPT = os.getenv('EXPECTTEST_ACCEPT', '0') == '1'

random_state_numpy = np.random.RandomState(0)


def get_available_classification_models():
# TODO add a registration mechanism to torchvision.models
Expand Down Expand Up @@ -84,7 +87,7 @@ def _assert_expected(output, name, prec):
else:
expected = torch.load(expected_file)
rtol = atol = prec
torch.testing.assert_close(output, expected, rtol=rtol, atol=atol, check_dtype=False)
torch.testing.assert_close(output, expected, rtol=rtol, atol=atol, check_dtype=False, msg=f'for query {name}')


def _check_jit_scriptable(nn_module, args, unwrapper=None, skip=False):
Expand Down Expand Up @@ -193,7 +196,8 @@ 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),
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree i'll revert these!

'random_state_numpy': random_state_numpy,
},
'retinanet_resnet50_fpn': {
'num_classes': 20,
Expand Down
25 changes: 13 additions & 12 deletions test/test_transforms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

from common_utils import cycle_over, int_dtypes, float_dtypes, assert_equal

random_state_numpy = np.random.RandomState(0)

GRACE_HOPPER = get_file_path_2(
os.path.dirname(os.path.abspath(__file__)), 'assets', 'encode_jpeg', 'grace_hopper_517x606.jpg')
Expand Down Expand Up @@ -206,12 +207,12 @@ def test_to_tensor(self, channels):
output = trans(img)
torch.testing.assert_close(output, input_data)

ndarray = np.random.randint(low=0, high=255, size=(height, width, channels)).astype(np.uint8)
ndarray = random_state_numpy.randint(low=0, high=255, size=(height, width, channels)).astype(np.uint8)
output = trans(ndarray)
expected_output = ndarray.transpose((2, 0, 1)) / 255.0
torch.testing.assert_close(output.numpy(), expected_output, check_dtype=False)

ndarray = np.random.rand(height, width, channels).astype(np.float32)
ndarray = random_state_numpy.rand(height, width, channels).astype(np.float32)
output = trans(ndarray)
expected_output = ndarray.transpose((2, 0, 1))
torch.testing.assert_close(output.numpy(), expected_output, check_dtype=False)
Expand All @@ -227,20 +228,20 @@ def test_to_tensor_errors(self):
trans = transforms.ToTensor()

with pytest.raises(TypeError):
trans(np.random.rand(1, height, width).tolist())
trans(random_state_numpy.rand(1, height, width).tolist())

with pytest.raises(ValueError):
trans(np.random.rand(height))
trans(random_state_numpy.rand(height))

with pytest.raises(ValueError):
trans(np.random.rand(1, 1, height, width))
trans(random_state_numpy.rand(1, 1, height, width))

@pytest.mark.parametrize('dtype', [torch.float16, torch.float, torch.double])
def test_to_tensor_with_other_default_dtypes(self, dtype):
current_def_dtype = torch.get_default_dtype()

t = transforms.ToTensor()
np_arr = np.random.randint(0, 255, (32, 32, 3), dtype=np.uint8)
np_arr = random_state_numpy.randint(0, 255, (32, 32, 3), dtype=np.uint8)
img = Image.fromarray(np_arr)

torch.set_default_dtype(dtype)
Expand All @@ -259,13 +260,13 @@ def test_pil_to_tensor(self, channels):
output = trans(img)
torch.testing.assert_close(input_data, output)

input_data = np.random.randint(low=0, high=255, size=(height, width, channels)).astype(np.uint8)
input_data = random_state_numpy.randint(low=0, high=255, size=(height, width, channels)).astype(np.uint8)
img = transforms.ToPILImage()(input_data)
output = trans(img)
expected_output = input_data.transpose((2, 0, 1))
torch.testing.assert_close(output.numpy(), expected_output)

input_data = torch.as_tensor(np.random.rand(channels, height, width).astype(np.float32))
input_data = torch.as_tensor(random_state_numpy.rand(channels, height, width).astype(np.float32))
img = transforms.ToPILImage()(input_data) # CHW -> HWC and (* 255).byte()
output = trans(img) # HWC -> CHW
expected_output = (input_data * 255).byte()
Expand All @@ -282,10 +283,10 @@ def test_pil_to_tensor_errors(self):
trans = transforms.PILToTensor()

with pytest.raises(TypeError):
trans(np.random.rand(1, height, width).tolist())
trans(random_state_numpy.rand(1, height, width).tolist())

with pytest.raises(TypeError):
trans(np.random.rand(1, height, width))
trans(random_state_numpy.rand(1, height, width))


def test_randomresized_params():
Expand Down Expand Up @@ -1183,7 +1184,7 @@ def test_random_grayscale():
random_state = random.getstate()
random.seed(42)
x_shape = [2, 2, 3]
x_np = np.random.randint(0, 256, x_shape, np.uint8)
x_np = random_state_numpy.randint(0, 256, x_shape, np.uint8)
x_pil = Image.fromarray(x_np, mode='RGB')
x_pil_2 = x_pil.convert('L')
gray_np = np.array(x_pil_2)
Expand All @@ -1206,7 +1207,7 @@ def test_random_grayscale():
random_state = random.getstate()
random.seed(42)
x_shape = [2, 2, 3]
x_np = np.random.randint(0, 256, x_shape, np.uint8)
x_np = random_state_numpy.randint(0, 256, x_shape, np.uint8)
x_pil = Image.fromarray(x_np, mode='RGB')
x_pil_2 = x_pil.convert('L')
gray_np = np.array(x_pil_2)
Expand Down
4 changes: 3 additions & 1 deletion test/test_transforms_video.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
warnings.simplefilter("always")
import torchvision.transforms._transforms_video as transforms

random_state_numpy = np.random.RandomState(0)


class TestVideoTransforms():

Expand Down Expand Up @@ -131,7 +133,7 @@ def test_to_tensor_video(self):
trans = transforms.ToTensorVideo()

with pytest.raises(TypeError):
trans(np.random.rand(numFrames, height, width, 1).tolist())
trans(random_state_numpy.rand(numFrames, height, width, 1).tolist())
trans(torch.rand((numFrames, height, width, 1), dtype=torch.float))

with pytest.raises(ValueError):
Expand Down
8 changes: 6 additions & 2 deletions torchvision/models/inception.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from collections import namedtuple
import warnings
import torch
import numpy as np
from torch import nn, Tensor
import torch.nn.functional as F
from .._internally_replaced_utils import load_state_dict_from_url
Expand Down Expand Up @@ -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(),
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

) -> None:
super(Inception3, self).__init__()
if inception_blocks is None:
Expand Down Expand Up @@ -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),
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@fmassa fmassa Aug 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

@vmoens vmoens Aug 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@NicolasHug NicolasHug Aug 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

dtype=m.weight.dtype)
values = values.view(m.weight.size())
with torch.no_grad():
m.weight.copy_(values)
Expand Down
5 changes: 3 additions & 2 deletions torchvision/models/quantization/inception.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ def __init__(self, *args, **kwargs):


class QuantizableInception3(inception_module.Inception3):
def __init__(self, num_classes=1000, aux_logits=True, transform_input=False):
def __init__(self, num_classes=1000, aux_logits=True, transform_input=False, **kwargs):
super(QuantizableInception3, self).__init__(
num_classes=num_classes,
aux_logits=aux_logits,
Expand All @@ -190,7 +190,8 @@ def __init__(self, num_classes=1000, aux_logits=True, transform_input=False):
QuantizableInceptionD,
QuantizableInceptionE,
QuantizableInceptionAux
]
],
**kwargs
)
self.quant = torch.quantization.QuantStub()
self.dequant = torch.quantization.DeQuantStub()
Expand Down