From 8092eed84662051aae168c36634152c21cd5fd81 Mon Sep 17 00:00:00 2001 From: Michael Osthege Date: Mon, 9 Nov 2020 16:54:30 +0100 Subject: [PATCH 01/14] change tests to expect scalar results for (1,) shapes + by using more pytest.mark.parametrize, the overall number of overall tests increases, but the result should be easier to diagnose + an informative assert error message was added --- pymc3/tests/test_distributions_random.py | 96 +++++++----------------- 1 file changed, 29 insertions(+), 67 deletions(-) diff --git a/pymc3/tests/test_distributions_random.py b/pymc3/tests/test_distributions_random.py index b15eb9b00e..fb3fb1548a 100644 --- a/pymc3/tests/test_distributions_random.py +++ b/pymc3/tests/test_distributions_random.py @@ -204,6 +204,7 @@ def setup_method(self, *args, **kwargs): self.model = pm.Model() def get_random_variable(self, shape, with_vector_params=False, name=None): + """ Creates a RandomVariable of the parametrized distribution. """ if with_vector_params: params = { key: value * np.ones(self.shape, dtype=np.dtype(type(value))) @@ -226,79 +227,40 @@ def get_random_variable(self, shape, with_vector_params=False, name=None): @staticmethod def sample_random_variable(random_variable, size): + """ Draws samples from a RandomVariable using its .random() method. """ try: return random_variable.random(size=size) except AttributeError: return random_variable.distribution.random(size=size) - @pytest.mark.parametrize("size", [None, 5, (4, 5)], ids=str) - def test_scalar_parameter_shape(self, size): - rv = self.get_random_variable(None) - if size is None: - expected = (1,) - else: - expected = np.atleast_1d(size).tolist() - actual = np.atleast_1d(self.sample_random_variable(rv, size)).shape - assert tuple(expected) == actual - - @pytest.mark.parametrize("size", [None, 5, (4, 5)], ids=str) - def test_scalar_shape(self, size): - shape = 10 + @pytest.mark.parametrize("size", [None, (), 1, (1,), 5, (4, 5)], ids=str) + @pytest.mark.parametrize("shape", [None, ()], ids=str) + def test_scalar_distribution_shape(self, shape, size): + """ Draws samples of different [size] from a scalar [shape] RV. """ rv = self.get_random_variable(shape) - - if size is None: - expected = [] - else: - expected = np.atleast_1d(size).tolist() - expected.append(shape) - actual = np.atleast_1d(self.sample_random_variable(rv, size)).shape - assert tuple(expected) == actual - - @pytest.mark.parametrize("size", [None, 5, (4, 5)], ids=str) - def test_parameters_1d_shape(self, size): - rv = self.get_random_variable(self.shape, with_vector_params=True) - if size is None: - expected = [] - else: - expected = np.atleast_1d(size).tolist() - expected.append(self.shape) - actual = self.sample_random_variable(rv, size).shape - assert tuple(expected) == actual - - @pytest.mark.parametrize("size", [None, 5, (4, 5)], ids=str) - def test_broadcast_shape(self, size): - broadcast_shape = (2 * self.shape, self.shape) - rv = self.get_random_variable(broadcast_shape, with_vector_params=True) - if size is None: - expected = [] - else: - expected = np.atleast_1d(size).tolist() - expected.extend(broadcast_shape) - actual = np.atleast_1d(self.sample_random_variable(rv, size)).shape - assert tuple(expected) == actual - - @pytest.mark.parametrize( - "shape", [(), (1,), (1, 1), (1, 2), (10, 10, 1), (10, 10, 2)], ids=str - ) - def test_different_shapes_and_sample_sizes(self, shape): - prefix = self.distribution.__name__ - - rv = self.get_random_variable(shape, name=f"{prefix}_{shape}") - for size in (None, 1, 5, (4, 5)): - if size is None: - s = [] - else: - try: - s = list(size) - except TypeError: - s = [size] - if s == [1]: - s = [] - if shape not in ((), (1,)): - s.extend(shape) - e = tuple(s) - a = self.sample_random_variable(rv, size).shape - assert e == a + expected = () if size in {None, ()} else tuple(np.atleast_1d(size)) + actual = np.shape(self.sample_random_variable(rv, size)) + assert expected == actual, f"Sample size {size} from {shape}-shaped RV had shape {actual}. Expected: {expected}" + + @pytest.mark.parametrize("size", [None, ()], ids=str) + @pytest.mark.parametrize("shape", [None, (), (1,), (1, 1), (1, 2), (10, 11, 1), (9, 10, 2)], ids=str) + def test_scalar_sample_shape(self, shape, size): + """ Draws samples of scalar [size] from a [shape] RV. """ + rv = self.get_random_variable(shape) + expected = () if shape in {None, ()} else tuple(np.atleast_1d(shape)) + actual = np.shape(self.sample_random_variable(rv, size)) + assert expected == actual, f"Sample size {size} from {shape}-shaped RV had shape {actual}. Expected: {expected}" + + @pytest.mark.parametrize("size", [None, 3, (4, 5)], ids=str) + @pytest.mark.parametrize("shape", [None, 1, (10, 11, 1)], ids=str) + def test_vector_params(self, shape, size): + shape = self.shape + rv = self.get_random_variable(shape, with_vector_params=True) + exp_shape = () if shape in {None, ()} else tuple(np.atleast_1d(shape)) + exp_size = () if size in {None, ()} else tuple(np.atleast_1d(size)) + expected = exp_size + exp_shape + actual = np.shape(self.sample_random_variable(rv, size)) + assert expected == actual, f"Sample size {size} from {shape}-shaped RV had shape {actual}. Expected: {expected}" class TestGaussianRandomWalk(BaseTestCases.BaseTestCase): From fbb6fe2a365bd4c0dfd25646ccd3f3422ae1b971 Mon Sep 17 00:00:00 2001 From: Michael Osthege Date: Mon, 9 Nov 2020 17:00:37 +0100 Subject: [PATCH 02/14] only treat () as scalar shapes closes #4206 --- pymc3/distributions/distribution.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pymc3/distributions/distribution.py b/pymc3/distributions/distribution.py index a32031ebb8..2799049ea3 100644 --- a/pymc3/distributions/distribution.py +++ b/pymc3/distributions/distribution.py @@ -943,11 +943,9 @@ def _draw_value(param, point=None, givens=None, size=None): def _is_one_d(dist_shape): - if hasattr(dist_shape, "dshape") and dist_shape.dshape in ((), (0,), (1,)): + if hasattr(dist_shape, "dshape") and dist_shape.dshape in { (), }: return True - elif hasattr(dist_shape, "shape") and dist_shape.shape in ((), (0,), (1,)): - return True - elif to_tuple(dist_shape) == (): + elif hasattr(dist_shape, "shape") and dist_shape.shape in { (), }: return True return False @@ -1069,6 +1067,7 @@ def generate_samples(generator, *args, **kwargs): len(samples.shape) > len(dist_shape) and samples.shape[-len(dist_shape) :] == dist_shape[-len(dist_shape) :] ): + raise ValueError(f"This SHOULD be unreachable code. DON'T MERGE UNTIL THIS ENTIRE BLOCK WAS REMOVED. {samples.shape}, {size_tup}") samples = samples.reshape(samples.shape[1:]) if ( @@ -1077,5 +1076,6 @@ def generate_samples(generator, *args, **kwargs): and samples.shape[-1] == 1 and (samples.shape != size_tup or size_tup == tuple() or size_tup == (1,)) ): + raise ValueError(f"This SHOULD be unreachable code. DON'T MERGE UNTIL THIS ENTIRE BLOCK WAS REMOVED. {samples.shape}, {size_tup}") samples = samples.reshape(samples.shape[:-1]) return np.asarray(samples) From 058920345365e145d283a56db4935e33566f62d6 Mon Sep 17 00:00:00 2001 From: Michael Osthege Date: Mon, 9 Nov 2020 17:09:50 +0100 Subject: [PATCH 03/14] fixes shape tests with new scalar behavior --- pymc3/distributions/discrete.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pymc3/distributions/discrete.py b/pymc3/distributions/discrete.py index fd2d399618..53babb28dc 100644 --- a/pymc3/distributions/discrete.py +++ b/pymc3/distributions/discrete.py @@ -972,7 +972,7 @@ def random(self, point=None, size=None): return generate_samples( random_choice, p=p, - broadcast_shape=p.shape[:-1] or (1,), + broadcast_shape=p.shape[:-1], dist_shape=self.shape, size=size, ) From d89f87928ec304f186aef45c0b902c42a262f908 Mon Sep 17 00:00:00 2001 From: Michael Osthege Date: Mon, 9 Nov 2020 17:50:42 +0100 Subject: [PATCH 04/14] fix BetaBinomial new scalar shape handling --- pymc3/distributions/discrete.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pymc3/distributions/discrete.py b/pymc3/distributions/discrete.py index 53babb28dc..4b183617d1 100644 --- a/pymc3/distributions/discrete.py +++ b/pymc3/distributions/discrete.py @@ -200,7 +200,7 @@ def __init__(self, alpha, beta, n, *args, **kwargs): self.mode = tt.cast(tround(alpha / (alpha + beta)), "int8") def _random(self, alpha, beta, n, size=None): - size = size or 1 + size = size or () p = stats.beta.rvs(a=alpha, b=beta, size=size).flatten() # Sometimes scipy.beta returns nan. Ugh. while np.any(np.isnan(p)): From 08caafa4dcbaff6762c7139f8bd1d541734844f9 Mon Sep 17 00:00:00 2001 From: Michael Osthege Date: Mon, 9 Nov 2020 18:09:19 +0100 Subject: [PATCH 05/14] remove sample reshaping --- pymc3/distributions/distribution.py | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/pymc3/distributions/distribution.py b/pymc3/distributions/distribution.py index 2799049ea3..655528cd58 100644 --- a/pymc3/distributions/distribution.py +++ b/pymc3/distributions/distribution.py @@ -983,6 +983,7 @@ def generate_samples(generator, *args, **kwargs): Any remaining args and kwargs are passed on to the generator function. """ dist_shape = kwargs.pop("dist_shape", ()) + # TODO: the following variable is no longer used !! one_d = _is_one_d(dist_shape) size = kwargs.pop("size", None) broadcast_shape = kwargs.pop("broadcast_shape", None) @@ -1059,23 +1060,5 @@ def generate_samples(generator, *args, **kwargs): samples = generator(size=dist_bcast_shape, *args, **kwargs) else: samples = generator(size=size_tup + dist_bcast_shape, *args, **kwargs) - samples = np.asarray(samples) - # reshape samples here - if samples.ndim > 0 and samples.shape[0] == 1 and size_tup == (1,): - if ( - len(samples.shape) > len(dist_shape) - and samples.shape[-len(dist_shape) :] == dist_shape[-len(dist_shape) :] - ): - raise ValueError(f"This SHOULD be unreachable code. DON'T MERGE UNTIL THIS ENTIRE BLOCK WAS REMOVED. {samples.shape}, {size_tup}") - samples = samples.reshape(samples.shape[1:]) - - if ( - one_d - and samples.ndim > 0 - and samples.shape[-1] == 1 - and (samples.shape != size_tup or size_tup == tuple() or size_tup == (1,)) - ): - raise ValueError(f"This SHOULD be unreachable code. DON'T MERGE UNTIL THIS ENTIRE BLOCK WAS REMOVED. {samples.shape}, {size_tup}") - samples = samples.reshape(samples.shape[:-1]) return np.asarray(samples) From 21de4465151944192457c17c2578a1237c13fa47 Mon Sep 17 00:00:00 2001 From: Michael Osthege Date: Mon, 9 Nov 2020 18:10:46 +0100 Subject: [PATCH 06/14] adaptions for timeseries distributions --- pymc3/tests/test_distributions_random.py | 50 +++++++++++++++--------- 1 file changed, 32 insertions(+), 18 deletions(-) diff --git a/pymc3/tests/test_distributions_random.py b/pymc3/tests/test_distributions_random.py index fb3fb1548a..007834704b 100644 --- a/pymc3/tests/test_distributions_random.py +++ b/pymc3/tests/test_distributions_random.py @@ -198,6 +198,12 @@ def test_blocking_context(self): class BaseTestCases: class BaseTestCase(SeededTest): shape = 5 + # the following are the default values of the distribution that take effect + # when the parametrized shape/size in the test case is None. + # For every distribution that defaults to non-scalar shapes they must be + # specified by the inheriting Test class. example: TestGaussianRandomWalk + default_shape = () + default_size = () def setup_method(self, *args, **kwargs): super().setup_method(*args, **kwargs) @@ -215,30 +221,39 @@ def get_random_variable(self, shape, with_vector_params=False, name=None): if name is None: name = self.distribution.__name__ with self.model: - if shape is None: - return self.distribution(name, transform=None, **params) - else: - try: + try: + if shape is None: + # in the test case parametrization "None" means "no specified (default)" + return self.distribution(name, transform=None, **params) + else: return self.distribution(name, shape=shape, transform=None, **params) - except TypeError: - if np.sum(np.atleast_1d(shape)) == 0: - pytest.skip("Timeseries must have positive shape") - raise + except TypeError: + if np.sum(np.atleast_1d(shape)) == 0: + pytest.skip("Timeseries must have positive shape") + raise @staticmethod def sample_random_variable(random_variable, size): """ Draws samples from a RandomVariable using its .random() method. """ try: - return random_variable.random(size=size) + if size is None: + return random_variable.random() + else: + return random_variable.random(size=size) except AttributeError: - return random_variable.distribution.random(size=size) + if size is None: + return random_variable.distribution.random() + else: + return random_variable.distribution.random(size=size) @pytest.mark.parametrize("size", [None, (), 1, (1,), 5, (4, 5)], ids=str) @pytest.mark.parametrize("shape", [None, ()], ids=str) def test_scalar_distribution_shape(self, shape, size): """ Draws samples of different [size] from a scalar [shape] RV. """ rv = self.get_random_variable(shape) - expected = () if size in {None, ()} else tuple(np.atleast_1d(size)) + exp_shape = self.default_shape if shape is None else tuple(np.atleast_1d(shape)) + exp_size = self.default_size if size is None else tuple(np.atleast_1d(size)) + expected = exp_size + exp_shape actual = np.shape(self.sample_random_variable(rv, size)) assert expected == actual, f"Sample size {size} from {shape}-shaped RV had shape {actual}. Expected: {expected}" @@ -247,7 +262,9 @@ def test_scalar_distribution_shape(self, shape, size): def test_scalar_sample_shape(self, shape, size): """ Draws samples of scalar [size] from a [shape] RV. """ rv = self.get_random_variable(shape) - expected = () if shape in {None, ()} else tuple(np.atleast_1d(shape)) + exp_shape = self.default_shape if shape is None else tuple(np.atleast_1d(shape)) + exp_size = self.default_size if size is None else tuple(np.atleast_1d(size)) + expected = exp_size + exp_shape actual = np.shape(self.sample_random_variable(rv, size)) assert expected == actual, f"Sample size {size} from {shape}-shaped RV had shape {actual}. Expected: {expected}" @@ -256,8 +273,8 @@ def test_scalar_sample_shape(self, shape, size): def test_vector_params(self, shape, size): shape = self.shape rv = self.get_random_variable(shape, with_vector_params=True) - exp_shape = () if shape in {None, ()} else tuple(np.atleast_1d(shape)) - exp_size = () if size in {None, ()} else tuple(np.atleast_1d(size)) + exp_shape = self.default_shape if shape is None else tuple(np.atleast_1d(shape)) + exp_size = self.default_size if size is None else tuple(np.atleast_1d(size)) expected = exp_size + exp_shape actual = np.shape(self.sample_random_variable(rv, size)) assert expected == actual, f"Sample size {size} from {shape}-shaped RV had shape {actual}. Expected: {expected}" @@ -266,10 +283,7 @@ def test_vector_params(self, shape, size): class TestGaussianRandomWalk(BaseTestCases.BaseTestCase): distribution = pm.GaussianRandomWalk params = {"mu": 1.0, "sigma": 1.0} - - @pytest.mark.xfail(reason="Supporting this makes a nasty API") - def test_broadcast_shape(self): - super().test_broadcast_shape() + default_shape = (1,) class TestNormal(BaseTestCases.BaseTestCase): From 309507170967de12749c3560da8631d4dd966d44 Mon Sep 17 00:00:00 2001 From: Michael Osthege Date: Tue, 10 Nov 2020 09:51:54 +0100 Subject: [PATCH 07/14] take out old (1,) shape handling --- pymc3/distributions/posterior_predictive.py | 2 -- pymc3/tests/test_shape_handling.py | 3 --- 2 files changed, 5 deletions(-) diff --git a/pymc3/distributions/posterior_predictive.py b/pymc3/distributions/posterior_predictive.py index 7a4ab43d30..c5d621318b 100644 --- a/pymc3/distributions/posterior_predictive.py +++ b/pymc3/distributions/posterior_predictive.py @@ -558,8 +558,6 @@ def random_sample( shape: Tuple[int, ...], ) -> np.ndarray: val = meth(point=point, size=size) - if size == 1: - val = np.expand_dims(val, axis=0) try: assert val.shape == (size,) + shape, ( "Sampling from random of %s yields wrong shape" % param diff --git a/pymc3/tests/test_shape_handling.py b/pymc3/tests/test_shape_handling.py index 50ed4a24fd..9708aa9fb3 100644 --- a/pymc3/tests/test_shape_handling.py +++ b/pymc3/tests/test_shape_handling.py @@ -211,9 +211,6 @@ def test_broadcast_dist_samples_to(self, samples_to_broadcast_to): def test_sample_generate_values(fixture_model, fixture_sizes): model, RVs = fixture_model size = to_tuple(fixture_sizes) - if size == (1,): - # Single draws are interpreted as scalars for backwards compatibility - size = tuple() with model: prior = pm.sample_prior_predictive(samples=fixture_sizes) for rv in RVs: From 2e38d89ab9fe882211081128c17b6c8ef117c293 Mon Sep 17 00:00:00 2001 From: Michael Osthege Date: Wed, 11 Nov 2020 13:01:08 +0100 Subject: [PATCH 08/14] black formatting --- pymc3/distributions/distribution.py | 4 ++-- pymc3/tests/test_distributions_random.py | 16 ++++++++++++---- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/pymc3/distributions/distribution.py b/pymc3/distributions/distribution.py index 655528cd58..b419fe9b8b 100644 --- a/pymc3/distributions/distribution.py +++ b/pymc3/distributions/distribution.py @@ -943,9 +943,9 @@ def _draw_value(param, point=None, givens=None, size=None): def _is_one_d(dist_shape): - if hasattr(dist_shape, "dshape") and dist_shape.dshape in { (), }: + if hasattr(dist_shape, "dshape") and dist_shape.dshape in {()}: return True - elif hasattr(dist_shape, "shape") and dist_shape.shape in { (), }: + elif hasattr(dist_shape, "shape") and dist_shape.shape in {()}: return True return False diff --git a/pymc3/tests/test_distributions_random.py b/pymc3/tests/test_distributions_random.py index 007834704b..d46fc317f1 100644 --- a/pymc3/tests/test_distributions_random.py +++ b/pymc3/tests/test_distributions_random.py @@ -255,10 +255,14 @@ def test_scalar_distribution_shape(self, shape, size): exp_size = self.default_size if size is None else tuple(np.atleast_1d(size)) expected = exp_size + exp_shape actual = np.shape(self.sample_random_variable(rv, size)) - assert expected == actual, f"Sample size {size} from {shape}-shaped RV had shape {actual}. Expected: {expected}" + assert ( + expected == actual + ), f"Sample size {size} from {shape}-shaped RV had shape {actual}. Expected: {expected}" @pytest.mark.parametrize("size", [None, ()], ids=str) - @pytest.mark.parametrize("shape", [None, (), (1,), (1, 1), (1, 2), (10, 11, 1), (9, 10, 2)], ids=str) + @pytest.mark.parametrize( + "shape", [None, (), (1,), (1, 1), (1, 2), (10, 11, 1), (9, 10, 2)], ids=str + ) def test_scalar_sample_shape(self, shape, size): """ Draws samples of scalar [size] from a [shape] RV. """ rv = self.get_random_variable(shape) @@ -266,7 +270,9 @@ def test_scalar_sample_shape(self, shape, size): exp_size = self.default_size if size is None else tuple(np.atleast_1d(size)) expected = exp_size + exp_shape actual = np.shape(self.sample_random_variable(rv, size)) - assert expected == actual, f"Sample size {size} from {shape}-shaped RV had shape {actual}. Expected: {expected}" + assert ( + expected == actual + ), f"Sample size {size} from {shape}-shaped RV had shape {actual}. Expected: {expected}" @pytest.mark.parametrize("size", [None, 3, (4, 5)], ids=str) @pytest.mark.parametrize("shape", [None, 1, (10, 11, 1)], ids=str) @@ -277,7 +283,9 @@ def test_vector_params(self, shape, size): exp_size = self.default_size if size is None else tuple(np.atleast_1d(size)) expected = exp_size + exp_shape actual = np.shape(self.sample_random_variable(rv, size)) - assert expected == actual, f"Sample size {size} from {shape}-shaped RV had shape {actual}. Expected: {expected}" + assert ( + expected == actual + ), f"Sample size {size} from {shape}-shaped RV had shape {actual}. Expected: {expected}" class TestGaussianRandomWalk(BaseTestCases.BaseTestCase): From eb0fcf86e51d61eda0642da55cc6515a752abc19 Mon Sep 17 00:00:00 2001 From: Michael Osthege Date: Sun, 13 Dec 2020 23:55:57 +0100 Subject: [PATCH 09/14] raise ValueError when Distribution is initialized with a 0-length dimension --- pymc3/distributions/distribution.py | 6 ++++++ pymc3/tests/test_distributions_random.py | 8 ++++++++ 2 files changed, 14 insertions(+) diff --git a/pymc3/distributions/distribution.py b/pymc3/distributions/distribution.py index 23749f1c47..42125777f9 100644 --- a/pymc3/distributions/distribution.py +++ b/pymc3/distributions/distribution.py @@ -107,6 +107,12 @@ def __new__(cls, name, *args, **kwargs): dims = (dims,) shape = model.shape_from_dims(dims) + # failsafe against 0-shapes + if shape is not None and any(np.atleast_1d(shape) <= 0): + raise ValueError( + f"Distribution initialized with invalid shape {shape}. This is not allowed." + ) + # Some distributions do not accept shape=None if has_shape or shape is not None: dist = cls.dist(*args, **kwargs, shape=shape) diff --git a/pymc3/tests/test_distributions_random.py b/pymc3/tests/test_distributions_random.py index 186471bfb3..34f07ff3a8 100644 --- a/pymc3/tests/test_distributions_random.py +++ b/pymc3/tests/test_distributions_random.py @@ -264,6 +264,9 @@ def test_scalar_distribution_shape(self, shape, size): assert ( expected == actual ), f"Sample size {size} from {shape}-shaped RV had shape {actual}. Expected: {expected}" + # check that negative size raises an error + with pytest.raises(ValueError, match="not allowed"): + rv.random(size=-2) @pytest.mark.parametrize("size", [None, ()], ids=str) @pytest.mark.parametrize( @@ -293,6 +296,11 @@ def test_vector_params(self, shape, size): expected == actual ), f"Sample size {size} from {shape}-shaped RV had shape {actual}. Expected: {expected}" + @pytest.mark.parametrize("shape", [-2, 0, (0,), (2, 0), (5, 0, 3)]) + def test_shape_error_on_zero_shape_rv(self, shape): + with pytest.raises(ValueError, match="not allowed"): + self.get_random_variable(shape) + class TestGaussianRandomWalk(BaseTestCases.BaseTestCase): distribution = pm.GaussianRandomWalk From 7eb78a716804de38b1411f8c42349ff96b3b4068 Mon Sep 17 00:00:00 2001 From: Michael Osthege Date: Mon, 14 Dec 2020 11:47:11 +0100 Subject: [PATCH 10/14] Don't match ValueError message because some distributions use different texts --- pymc3/tests/test_distributions_random.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pymc3/tests/test_distributions_random.py b/pymc3/tests/test_distributions_random.py index 34f07ff3a8..7baf3e3999 100644 --- a/pymc3/tests/test_distributions_random.py +++ b/pymc3/tests/test_distributions_random.py @@ -265,8 +265,10 @@ def test_scalar_distribution_shape(self, shape, size): expected == actual ), f"Sample size {size} from {shape}-shaped RV had shape {actual}. Expected: {expected}" # check that negative size raises an error - with pytest.raises(ValueError, match="not allowed"): - rv.random(size=-2) + with pytest.raises(ValueError): + self.sample_random_variable(rv, size=-2) + with pytest.raises(ValueError): + self.sample_random_variable(rv, size=(3, -2)) @pytest.mark.parametrize("size", [None, ()], ids=str) @pytest.mark.parametrize( From 3ff49594d89c303eff947459c45ba1590b9e66b5 Mon Sep 17 00:00:00 2001 From: Michael Osthege Date: Mon, 14 Dec 2020 16:12:14 +0100 Subject: [PATCH 11/14] Update releasenotes and apply review feedback --- RELEASE-NOTES.md | 11 ++++++++++- pymc3/distributions/distribution.py | 10 ---------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md index 2a7b8f5796..106e4c418e 100644 --- a/RELEASE-NOTES.md +++ b/RELEASE-NOTES.md @@ -1,11 +1,20 @@ # Release Notes -## PyMC3 3.10.1 (on deck) +## PyMC3 3.11.0 (on deck) +This release breaks some APIs w.r.t. `3.10.0`. +It also brings some dreadfully awaited fixes, so be sure to go through the changes below. +(Or latest when you run into problems.) ### Maintenance +- Changed shape behavior: __No longer collapse length 1 vector shape into scalars.__ (see [#4206](https://github.com/pymc-devs/pymc3/issue/4206) and [#4214](https://github.com/pymc-devs/pymc3/pull/4214)) + - __Applies to random variables and also the `.random(size=...)` kwarg!__ + - To create scalar variables you must now use `shape=None` or `shape=()`. + - __`shape=(1,)` and `shape=1` now become vectors.__ Previously they were collapsed into scalars + - 0-length dimensions are now ruled illegal for random variables and raise a `ValueError`. - Fixed bug whereby partial traces returns after keyboard interrupt during parallel sampling had fewer draws than would've been available [#4318](https://github.com/pymc-devs/pymc3/pull/4318) - Make `sample_shape` same across all contexts in `draw_values` (see [#4305](https://github.com/pymc-devs/pymc3/pull/4305)). - Removed `theanof.set_theano_config` because it illegally touched Theano's privates (see [#4329](https://github.com/pymc-devs/pymc3/pull/4329)). +- In `sample_prior_predictive` the `vars` kwarg was removed in favor of `var_names` (see [#4327](https://github.com/pymc-devs/pymc3/pull/4327)) ## PyMC3 3.10.0 (7 December 2020) diff --git a/pymc3/distributions/distribution.py b/pymc3/distributions/distribution.py index 42125777f9..2ab840c536 100644 --- a/pymc3/distributions/distribution.py +++ b/pymc3/distributions/distribution.py @@ -1007,14 +1007,6 @@ def _draw_value(param, point=None, givens=None, size=None): raise ValueError("Unexpected type in draw_value: %s" % type(param)) -def _is_one_d(dist_shape): - if hasattr(dist_shape, "dshape") and dist_shape.dshape in {()}: - return True - elif hasattr(dist_shape, "shape") and dist_shape.shape in {()}: - return True - return False - - def generate_samples(generator, *args, **kwargs): """Generate samples from the distribution of a random variable. @@ -1048,8 +1040,6 @@ def generate_samples(generator, *args, **kwargs): Any remaining args and kwargs are passed on to the generator function. """ dist_shape = kwargs.pop("dist_shape", ()) - # TODO: the following variable is no longer used !! - one_d = _is_one_d(dist_shape) size = kwargs.pop("size", None) broadcast_shape = kwargs.pop("broadcast_shape", None) not_broadcast_kwargs = kwargs.pop("not_broadcast_kwargs", None) From 1652e4b7ecb7b3e9ad165ad63385da5dabc5b546 Mon Sep 17 00:00:00 2001 From: Sayam Kumar Date: Fri, 18 Dec 2020 13:30:02 +0530 Subject: [PATCH 12/14] Removed xfail tests while testing mvnormal random --- pymc3/tests/test_distributions_random.py | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/pymc3/tests/test_distributions_random.py b/pymc3/tests/test_distributions_random.py index 7baf3e3999..35e8710df1 100644 --- a/pymc3/tests/test_distributions_random.py +++ b/pymc3/tests/test_distributions_random.py @@ -1571,7 +1571,7 @@ def test_Triangular( assert prior["target"].shape == (prior_samples,) + shape -def generate_shapes(include_params=False, xfail=False): +def generate_shapes(include_params=False): # fmt: off mudim_as_event = [ [None, 1, 3, 10, (10, 3), 100], @@ -1590,20 +1590,13 @@ def generate_shapes(include_params=False, xfail=False): del mudim_as_event[-1] del mudim_as_dist[-1] data = itertools.chain(itertools.product(*mudim_as_event), itertools.product(*mudim_as_dist)) - if xfail: - data = list(data) - for index in range(len(data)): - if data[index][0] in (None, 1): - data[index] = pytest.param( - *data[index], marks=pytest.mark.xfail(reason="wait for PR #4214") - ) return data class TestMvNormal(SeededTest): @pytest.mark.parametrize( ["sample_shape", "dist_shape", "mu_shape", "param"], - generate_shapes(include_params=True, xfail=False), + generate_shapes(include_params=True), ids=str, ) def test_with_np_arrays(self, sample_shape, dist_shape, mu_shape, param): @@ -1613,7 +1606,7 @@ def test_with_np_arrays(self, sample_shape, dist_shape, mu_shape, param): @pytest.mark.parametrize( ["sample_shape", "dist_shape", "mu_shape"], - generate_shapes(include_params=False, xfail=True), + generate_shapes(include_params=False), ids=str, ) def test_with_chol_rv(self, sample_shape, dist_shape, mu_shape): @@ -1630,7 +1623,7 @@ def test_with_chol_rv(self, sample_shape, dist_shape, mu_shape): @pytest.mark.parametrize( ["sample_shape", "dist_shape", "mu_shape"], - generate_shapes(include_params=False, xfail=True), + generate_shapes(include_params=False), ids=str, ) def test_with_cov_rv(self, sample_shape, dist_shape, mu_shape): From 3e52058bbf814e3496479829f838e439d85f8b6e Mon Sep 17 00:00:00 2001 From: Michael Osthege Date: Sun, 20 Dec 2020 14:55:44 +0100 Subject: [PATCH 13/14] Take out size=1 special case from MixtureSameFamily --- pymc3/distributions/mixture.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/pymc3/distributions/mixture.py b/pymc3/distributions/mixture.py index dee5a21c66..51b35812a7 100644 --- a/pymc3/distributions/mixture.py +++ b/pymc3/distributions/mixture.py @@ -870,12 +870,6 @@ def random(self, point=None, size=None): # The `samples` array still has the `mixture_axis`, so we must remove it: output = samples[(..., 0) + (slice(None),) * len(event_shape)] - - # Final oddity: if size == 1, pymc3 defaults to reducing the sample_shape dimension - # We do this to stay consistent with the rest of the package even though - # we shouldn't have to do it. - if size == 1: - output = output[0] return output def _distr_parameters_for_repr(self): From 39ac694cc868c4be5ce5efece9ee1351ea9e57c2 Mon Sep 17 00:00:00 2001 From: Michael Osthege Date: Sun, 20 Dec 2020 17:25:27 +0100 Subject: [PATCH 14/14] Use more professional terminology Co-authored-by: Thomas Wiecki --- RELEASE-NOTES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md index ab3cb8621d..32ae60e90c 100644 --- a/RELEASE-NOTES.md +++ b/RELEASE-NOTES.md @@ -13,7 +13,7 @@ It also brings some dreadfully awaited fixes, so be sure to go through the chang - __`shape=(1,)` and `shape=1` now become vectors.__ Previously they were collapsed into scalars - 0-length dimensions are now ruled illegal for random variables and raise a `ValueError`. - In `sample_prior_predictive` the `vars` kwarg was removed in favor of `var_names` (see [#4327](https://github.com/pymc-devs/pymc3/pull/4327)). -- Removed `theanof.set_theano_config` because it illegally touched Theano's privates (see [#4329](https://github.com/pymc-devs/pymc3/pull/4329)). +- Removed `theanof.set_theano_config` because it illegally changed Theano's internal state (see [#4329](https://github.com/pymc-devs/pymc3/pull/4329)). ### New Features - `OrderedProbit` distribution added (see [#4232](https://github.com/pymc-devs/pymc3/pull/4232)).