V4 update test framework for distributions random method 2nd attempt #4608

matteo-pallini · 2021-04-03T11:54:02Z

The recent distributions refactoring (#4508 and #4548) moves the logic to
get a random variable from a distribution to aesara.
This relies on numpy and scipy random variables implementation.
Given this change the main thing worth testing on the PyMC side is that
the PyMC parametrization is sensible given the one on the Aesara side
(effectively the numpy/scipy one)

More details can be found on issue #4554

this is the second attempt for doing this change. The first one can be found here:
WIP: V4 update test framework for distributions random method #4580

what are the (breaking) changes that this PR makes? No breaking changes, only fixing currently broken tests
are the changes—especially new features—covered by tests and docstrings? This change is only affecting tests
Is there any other test not relevant anymore given the refactoring?

ricardoV94

Looks like a good start. We should also check that .eval() actually works even if just for a single sample.

I think @brandonwillard had some code in the original issue asserting that the numbers generated by our distribution and the scipy one were the same when given the same random seeds. That feels like a stronger test.

PS: Checking that a dozen or so samples match should be enough as we already asserted the right parameters are being used.

What do you think?

pymc3/tests/test_distributions_random.py

ricardoV94 · 2021-04-04T06:42:12Z

The exponential and gamma should now pass after #4576

matteo-pallini · 2021-04-04T09:20:46Z

The exponential and gamma should now pass after #4576

Lovely, thanks for flagging

matteo-pallini · 2021-04-04T11:42:16Z

Looks like a good start. We should also check that .eval() actually works even if just for a single sample.

I think @brandonwillard had some code in the original issue asserting that the numbers generated by our distribution and the scipy one were the same when given the same random seeds. That feels like a stronger test.

PS: Checking that a dozen or so samples match should be enough as we already asserted the right parameters are being used.

What do you think?

Sounds good. It's definitely a more robust test. Let me know if what added is in line with what you had in mind

pymc3/tests/test_distributions_random.py

ricardoV94 · 2021-04-04T13:14:08Z

I left some small comments. Overall I think it looks fine and should definitely cover the pymc <- -> aesara parametrizations. After this PR we might still need something extra for the random ops that are implemented on our side, but that might be better done separately.

ricardoV94 · 2021-04-04T13:17:24Z

Also, we can remove these lines from the previous PR: https://github.com/pymc-devs/pymc3/blob/7a7179276f8f60470dacc496d3c2add4b636e4a2/pymc3/tests/test_distributions_random.py#L1773-L1803

pymc3/tests/test_distributions_random.py

ricardoV94 · 2021-04-05T06:36:18Z

I think we are pretty much ready after you complete those final refactorings. Thanks for your work and patience.

Is there anything else you think should be changed?

matteo-pallini · 2021-04-05T09:37:56Z

I think we are pretty much ready after you complete those final refactorings. Thanks for your work and patience.

Is there anything else you think should be changed?

Happy to cover If this test framework would also cover well these future cases it could be worthwhile as well. Let's agree on the right way of doing it, if you are happy to. I left a comment with a potential approach on the thread above. I should probably thank you for your patience :-)

I should have addressed all the refactoring comments you left

The refactoring should make it possible testing both the distribution parametrization and sampled values according to need, as well as any other future test. More details on PR pymc-devs#4608

ricardoV94

Thanks for the updates. I left some general comments, let me know what you think :)

pymc3/tests/test_distributions_random.py

ricardoV94 · 2021-04-10T14:27:41Z

pymc3/tests/test_distributions_random.py

+            "expected_rv_op_params": {"mu": 1.5, "beta": 3.0},
+            "expected_dist": self._get_scipy_distribution("gumbel_r"),
+            "expected_dist_params": {"loc": 1.5, "scale": 3.0},
+            "size": 15,


Any reason why this needed size explicitly?

I guess I can remove it, given that the default value is 15 anyway. it's not needed, but it shows very explicitly how to define the size

I think it's fine to leave it out. Specially since the BaseTests parametrize size separately.

pymc3/tests/test_distributions_random.py

ricardoV94 · 2021-04-10T16:33:42Z

Sounds like you are on the right track.

I would focus on integrating the BaseTest with your framework and give it a more explicit name to make it obvious it is testing shapes of draws. If you find the inheriting class logic nicer, that would certainly be a plus.

After this it's perhaps time to get a second review, merge, and start using it with the newly refactored distributions. It will be more obvious whether more types of tests are needed or if these suffice.

The refactoring should make it possible testing both the distribution parametrization and sampled values according to need, as well as any other future test. More details on PR pymc-devs#4608

ricardoV94

Left some minor comments. The way the discrete_weibul_rng_fn is implemented looks a bit complicated. Do you think it might read better if we unpack that lambda to an explicit function?

Otherwise things LGTM!

The distributions refactoring moves the random variable sampling to aesara. This relies on numpy and scipy random variables implementation. So, now the only thing we care about testing is that the parametrization on the PyMC side is sendible given the one on the Aesara side (effectively the numpy/scipy one) More details can be found on issue pymc-devs#4554 pymc-devs#4554

More details can be found on issue pymc-devs#4554 pymc-devs#4554

Most of the random variable logic has been moved to aesara, as well as most of the relative tests. More details can be found on issue pymc-devs#4554

Bernoulli

Also mark test_categorical as expected to fail due to bug on aesara side. The bug is going to be fixed with 2.0.5 release, so we need to bump the version for categorical and the test to pass.

…lative bug-fix

- replace list of tuples with dict - rename 1 method - move pymc_dist as first argument in function call - replace list(params) with params.copy()

The refactoring should make it possible testing both the distribution parametrization and sampled values according to need, as well as any other future test. More details on PR pymc-devs#4608

ricardoV94

LGTM, much more readable and obvious how to apply to new distributions going into the future!

ricardoV94 · 2021-04-26T06:39:52Z

Thanks a lot @DRabbit17. Looking forward to your next PR!

matteo-pallini · 2021-04-26T08:04:03Z

Thanks for reviewing and helping in making the final version much better

* Update tests following distributions refactoring The distributions refactoring moves the random variable sampling to aesara. This relies on numpy and scipy random variables implementation. So, now the only thing we care about testing is that the parametrization on the PyMC side is sendible given the one on the Aesara side (effectively the numpy/scipy one) More details can be found on issue #4554 #4554 * Change tests for more refactored distributions. More details can be found on issue #4554 #4554 * Change tests for refactored distributions More details can be found on issue #4554 #4554 * Remove tests for random variable samples shape and size Most of the random variable logic has been moved to aesara, as well as most of the relative tests. More details can be found on issue #4554 * Fix test for half cauchy, renmae mv normal tests and add test for Bernoulli * Add test checking PyMC samples match the aesara ones Also mark test_categorical as expected to fail due to bug on aesara side. The bug is going to be fixed with 2.0.5 release, so we need to bump the version for categorical and the test to pass. * Move Aesara to 2.0.5 to include Gumbel distribution * Enamble exponential and gamma tests following bug-fix * Enable categorical test following aesara version bump to 2.0.5 and relative bug-fix * Few small cosmetic changes: - replace list of tuples with dict - rename 1 method - move pymc_dist as first argument in function call - replace list(params) with params.copy() * Remove redundant tests * Further refactoring The refactoring should make it possible testing both the distribution parametrization and sampled values according to need, as well as any other future test. More details on PR #4608 * Add size tests to new rv testing framework * Add tests for multivariate and for univariate multi-parameters * remove test already covered in aesara * fix few names * Remove "distribution" from test class names * Add discrete Weibull, improve Beta and some minor refactoring * Fix typos in checks naming and add sanity check Co-authored-by: Ricardo <[email protected]>

matteo-pallini changed the title ~~WIP: V4 update test framework for distributions random method 2nd attempt~~ V4 update test framework for distributions random method 2nd attempt Apr 3, 2021

This was referenced Apr 3, 2021

WIP: V4 update test framework for distributions random method #4580

Closed

Establish new test framework for the "random" behavior of Distributions #4554

Closed

ricardoV94 reviewed Apr 3, 2021

View reviewed changes

pymc3/tests/test_distributions_random.py Outdated Show resolved Hide resolved

pymc3/tests/test_distributions_random.py Outdated Show resolved Hide resolved

ricardoV94 reviewed Apr 3, 2021

View reviewed changes

pymc3/tests/test_distributions_random.py Outdated Show resolved Hide resolved

matteo-pallini force-pushed the v4-update-test-framework-for-distributions-random-method-2nd-attempt branch from 53dfb75 to ef7584e Compare April 4, 2021 11:44

ricardoV94 reviewed Apr 4, 2021

View reviewed changes

pymc3/tests/test_distributions_random.py Outdated Show resolved Hide resolved

ricardoV94 reviewed Apr 4, 2021

View reviewed changes

pymc3/tests/test_distributions_random.py Outdated Show resolved Hide resolved

ricardoV94 mentioned this pull request Apr 4, 2021

Refactor DiscreteWeibull #4615

Merged

ricardoV94 reviewed Apr 4, 2021

View reviewed changes

pymc3/tests/test_distributions_random.py Outdated Show resolved Hide resolved

matteo-pallini closed this Apr 5, 2021

matteo-pallini reopened this Apr 5, 2021

matteo-pallini force-pushed the v4-update-test-framework-for-distributions-random-method-2nd-attempt branch from 1ca93bc to 598d0db Compare April 5, 2021 13:29

ricardoV94 mentioned this pull request Apr 7, 2021

Reintroduce Bernoulli logitp parametrization #4620

Closed

6 tasks

ricardoV94 added the v4 label Apr 7, 2021

matteo-pallini force-pushed the v4-update-test-framework-for-distributions-random-method-2nd-attempt branch from 598d0db to 5e67189 Compare April 8, 2021 23:39

matteo-pallini force-pushed the v4-update-test-framework-for-distributions-random-method-2nd-attempt branch from 5e67189 to e4901ee Compare April 10, 2021 12:03

ricardoV94 reviewed Apr 10, 2021

View reviewed changes

matteo-pallini force-pushed the v4-update-test-framework-for-distributions-random-method-2nd-attempt branch from e4901ee to 69592c3 Compare April 11, 2021 10:23

ricardoV94 reviewed Apr 23, 2021

View reviewed changes

matteo-pallini and others added 18 commits April 24, 2021 00:00

Change tests for more refactored distributions.

45180b1

More details can be found on issue pymc-devs#4554 pymc-devs#4554

Change tests for refactored distributions

7fed128

More details can be found on issue pymc-devs#4554 pymc-devs#4554

Remove tests for random variable samples shape and size

6cb7a6b

Most of the random variable logic has been moved to aesara, as well as most of the relative tests. More details can be found on issue pymc-devs#4554

Fix test for half cauchy, renmae mv normal tests and add test for

fe5d7d9

Bernoulli

Add test checking PyMC samples match the aesara ones

6b576c4

Also mark test_categorical as expected to fail due to bug on aesara side. The bug is going to be fixed with 2.0.5 release, so we need to bump the version for categorical and the test to pass.

Move Aesara to 2.0.5 to include Gumbel distribution

b50e92f

Enamble exponential and gamma tests following bug-fix

78ac5ac

Enable categorical test following aesara version bump to 2.0.5 and re…

b7afa5d

…lative bug-fix

Few small cosmetic changes:

d6c3847

- replace list of tuples with dict - rename 1 method - move pymc_dist as first argument in function call - replace list(params) with params.copy()

Remove redundant tests

7b5899c

Further refactoring

b1c40ef

The refactoring should make it possible testing both the distribution parametrization and sampled values according to need, as well as any other future test. More details on PR pymc-devs#4608

Add size tests to new rv testing framework

a817a7e

Add tests for multivariate and for univariate multi-parameters

1c88e55

remove test already covered in aesara

bf68a3a

fix few names

55b4a0f

Remove "distribution" from test class names

706308e

Add discrete Weibull, improve Beta and some minor refactoring

3d28087

matteo-pallini force-pushed the v4-update-test-framework-for-distributions-random-method-2nd-attempt branch 3 times, most recently from 56253dc to c5661ba Compare April 24, 2021 09:26

Fix typos in checks naming and add sanity check

9b52e1b

matteo-pallini force-pushed the v4-update-test-framework-for-distributions-random-method-2nd-attempt branch from c5661ba to 9b52e1b Compare April 24, 2021 10:22

ricardoV94 approved these changes Apr 24, 2021

View reviewed changes

ricardoV94 mentioned this pull request Apr 26, 2021

Distribution class refactoring: an example using the Weibull distribution #4668

Closed

ricardoV94 merged commit 7af5b46 into pymc-devs:v4 Apr 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

V4 update test framework for distributions random method 2nd attempt #4608

V4 update test framework for distributions random method 2nd attempt #4608

matteo-pallini commented Apr 3, 2021 •

edited

Loading

ricardoV94 left a comment •

edited

Loading

ricardoV94 commented Apr 4, 2021

matteo-pallini commented Apr 4, 2021

matteo-pallini commented Apr 4, 2021 •

edited

Loading

ricardoV94 commented Apr 4, 2021

ricardoV94 commented Apr 4, 2021 •

edited

Loading

ricardoV94 commented Apr 5, 2021

matteo-pallini commented Apr 5, 2021 •

edited

Loading

ricardoV94 left a comment

ricardoV94 Apr 10, 2021

matteo-pallini Apr 10, 2021 •

edited

Loading

ricardoV94 Apr 10, 2021

ricardoV94 commented Apr 10, 2021

ricardoV94 left a comment

ricardoV94 left a comment

ricardoV94 commented Apr 26, 2021

matteo-pallini commented Apr 26, 2021

V4 update test framework for distributions random method 2nd attempt #4608

V4 update test framework for distributions random method 2nd attempt #4608

Conversation

matteo-pallini commented Apr 3, 2021 • edited Loading

ricardoV94 left a comment • edited Loading

Choose a reason for hiding this comment

ricardoV94 commented Apr 4, 2021

matteo-pallini commented Apr 4, 2021

matteo-pallini commented Apr 4, 2021 • edited Loading

ricardoV94 commented Apr 4, 2021

ricardoV94 commented Apr 4, 2021 • edited Loading

ricardoV94 commented Apr 5, 2021

matteo-pallini commented Apr 5, 2021 • edited Loading

ricardoV94 left a comment

Choose a reason for hiding this comment

ricardoV94 Apr 10, 2021

Choose a reason for hiding this comment

matteo-pallini Apr 10, 2021 • edited Loading

Choose a reason for hiding this comment

ricardoV94 Apr 10, 2021

Choose a reason for hiding this comment

ricardoV94 commented Apr 10, 2021

ricardoV94 left a comment

Choose a reason for hiding this comment

ricardoV94 left a comment

Choose a reason for hiding this comment

ricardoV94 commented Apr 26, 2021

matteo-pallini commented Apr 26, 2021

matteo-pallini commented Apr 3, 2021 •

edited

Loading

ricardoV94 left a comment •

edited

Loading

matteo-pallini commented Apr 4, 2021 •

edited

Loading

ricardoV94 commented Apr 4, 2021 •

edited

Loading

matteo-pallini commented Apr 5, 2021 •

edited

Loading

matteo-pallini Apr 10, 2021 •

edited

Loading