Refactor several distributions

[ricardoV94]

Couple more of distributions refactored to v4. This should cover all the ones that are to be implemented on Aesara, unless I missed something.

Depending on what your PR does, here are a few things you might want to address in the description:

• what are the (breaking) changes that this PR makes?
• important background, or details about the implementation
• are the changes—especially new features—covered by tests and docstrings?
• linting/style checks have been run
• consider adding/updating relevant example notebooks
• right before it's ready to merge, mention the PR in the RELEASE-NOTES.md

[ricardoV94]

@brandonwillard What do you think of my last commit as a solution for the BoundContinuous issue?

I moved the transform_logp inside pm.Distribution.__new__() instead of being inside model.create_value_var(). This allows to pass class attributes as optional arguments to transform_logp without cluttering the rv_op.

BTW I see this was before in the metaclass: https://github.com/pymc-devs/pymc3/blob/1e4163339f963d9728d0160e74d594f685d76b92/pymc3/distributions/distribution.py#L112-L120

Should we move it there or remove those commented lines?

[brandonwillard]

We don't want to start adding variables at the class level unnecessarily; instead, each class that wants to use an interval transform should just create its own in Distribution.__new__. That approach is class-level enough to accomplish the same thing, and it makes the transform-setup logic a little less spread out.

[twiecki]

=================================== FAILURES ===================================
_______________ TestMatchesScipy.test_beta_binomial_distribution _______________

self = <pymc3.tests.test_distributions.TestMatchesScipy object at 0x12affbeb0>

    @pytest.mark.xfail(condition=(aesara.config.floatX == "float32"), reason="Fails on float32")
    def test_beta_binomial_distribution(self):
>       self.checkd(
            BetaBinomial,
            Nat,
            {"alpha": Rplus, "beta": Rplus, "n": NatSmall},
        )

pymc3/tests/test_distributions.py:1501: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <pymc3.tests.test_distributions.TestMatchesScipy object at 0x12affbeb0>
distfam = <class 'pymc3.distributions.discrete.BetaBinomial'>
valuedomain = <pymc3.tests.test_distributions.Domain object at 0x12511d880>
vardomains = {'alpha': <pymc3.tests.test_distributions.Domain object at 0x12560c7f0>, 'beta': <pymc3.tests.test_distributions.Domain object at 0x12560c7f0>, 'n': <pymc3.tests.test_distributions.Domain object at 0x12511dfd0>}
checks = (<bound method TestMatchesScipy.check_int_to_1 of <pymc3.tests.test_distributions.TestMatchesScipy object at 0x12affbeb0>>,)
extra_args = {}

    def checkd(self, distfam, valuedomain, vardomains, checks=None, extra_args=None):
        if checks is None:
            checks = (self.check_int_to_1,)
    
        if extra_args is None:
            extra_args = {}
        m = build_model(distfam, valuedomain, vardomains, extra_args=extra_args)
        for check in checks:
>           check(m, m.named_vars["value"], valuedomain, vardomains)
E           AttributeError: 'tuple' object has no attribute 'named_vars'

pymc3/tests/test_distributions.py:863: AttributeError
=============================== warnings summary ===============================

[ricardoV94]

Yeah, it's using this checkd test that has not been refactored for V4. Just marked as xfail for now. Will fix later when refactoring the ZeroInflated distributions that make use of it as well.

[ricardoV94]

I added a skipif for the betabinomial test in case we are using scipy < 1.4.0 (which happens in the pytest workflow). The tests should still be covered in the arviz-compat workflow so it should be fine for now.

[brandonwillard]

Aside from renaming that transform field, it looks like test_distributions_random could be introducing a significant amount of testing redundancy. Regardless, we can merge this once the transform field name is updated.

[brandonwillard]

Since this field is very specific to upper and lower bounds, its name should reflect that. Currently, it seems like this field specifies which arguments are to be transformed, which is very misleading. Also, the values in this field are argument indices, so that should be made clearer somehow (e.g. upper_lower_bound_indices, trans_ul_bound_indices, etc.).

[ricardoV94]

Renamed to bound_args_indices

[brandonwillard]

Let's create an issue specifically for this before/after merging.

[ricardoV94]

Done: #4683

[twiecki]

Why 2? And shouldn't these be tuples instead of lists?

[ricardoV94]

Changed to tuples.

The arguments of the triangular RandomOp are in this order (lower, mode, upper), and so we need to specify that the first and last are the ones relevant for the transformation, hence (0, 2) (zero-based index)

[twiecki]

Why don't we need self here?

[ricardoV94]

Because these are used only as a sort of static-method by the _logp dispatcher here: https://github.com/pymc-devs/pymc3/blob/6c247638c506e4b3d6eff86ff24c5e98d34ae055/pymc3/distributions/distribution.py#L96-L102

We are not really using distribution instances anymore IIUC.

[twiecki]

What a curious pattern, thanks for explaining.

[twiecki]

This gets us a big step closer to merging v4, thanks @ricardoV94!