Add alternative parameterization to negative binomial distribution #4126

[ricardoV94]

Changes allow the specification of the NegativeBinomial distribution in terms of p (the probability of observing a success in each trial), and n (the target number of successes). This parametrization gives the likelihood for y, the number of failures observed until a target number of successes is reached.

I changed the docstrings and distribution initialization in line with other distribution that have multiple parametrizations.

I didn't figure out yet how the tests work, so I did not implement it (hence the WIP)! This should be straightforward, however, since the scipy negativebinomial distribution is implemented in terms of n and p. Any guidance in how to proceed is much appreciated!

Pinging @twiecki

Ref #4126

[codecov]

Codecov Report

Merging #4134 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4134   +/-   ##
=======================================
  Coverage   88.75%   88.75%           
=======================================
  Files          89       89           
  Lines       14037    14039    +2     
=======================================
+ Hits        12458    12461    +3     
+ Misses       1579     1578    -1     

Impacted Files	Coverage Δ
pymc3/distributions/discrete.py	97.14% <100.00%> (+0.16%)	⬆️
pymc3/plots/__init__.py	50.00% <0.00%> (ø)
pymc3/stats/__init__.py	92.00% <0.00%> (ø)
pymc3/step_methods/gibbs.py	40.00% <0.00%> (ø)
pymc3/distributions/bound.py	91.59% <0.00%> (ø)
pymc3/step_methods/sgmcmc.py	0.00% <0.00%> (ø)
pymc3/step_methods/slicer.py	95.45% <0.00%> (ø)
pymc3/step_methods/hmc/hmc.py	92.15% <0.00%> (ø)
pymc3/variational/__init__.py	100.00% <0.00%> (ø)
pymc3/distributions/mixture.py	88.47% <0.00%> (ø)
... and 18 more

[twiecki]

This is great -- thanks! Definitely tests are required, do you need help figuring that out?

[ricardoV94]

Yeah, I would need some help with that. I didn't find anything on the developer's guide about testing, and the test code is a bit opaque for me.

[AlexAndorra]

Thanks for the PR @ricardoV94 ! For the tests, I think you can take inspiration from the tests of Negative Binomial done with the current parametrization. Doing them with the new one should already be a good sample of use-cases

[ricardoV94]

@twiecki @AlexAndorra would the added test be enough?

[MarcoGorelli]

IMO there should also be a test which hits the various error messages you've added, e.g. when you set both alpha and n - is that something you'd be happy to add? Feel free to ping if you want/need help

[ricardoV94]

@MarcoGorelli That makes sense.

I am not sure how to do it. Could you direct me to a snippet where this type of test is implemented? For example, the Beta distribution also raises this type of ValueErrors, but I couldn't find if it is being tested anywhere.

[MarcoGorelli]

They do something similar in pymc3/tests/test_starting.py:

    with raises(ValueError, match=r"Some variables not in the model: \['x2', 'y2'\]"):
        starting.allinmodel([x2, y2], model1)
    with raises(ValueError, match=r"Some variables not in the model: \['x2'\]"):
        starting.allinmodel([x2, y1], model1)
    with raises(ValueError, match=r"Some variables not in the model: \['x2'\]"):
        starting.allinmodel([x2], model1)

where raises was imported from pytest (see https://docs.pytest.org/en/stable/assert.html for more on this)

[ricardoV94]

@MarcoGorelli Thanks for the input. I added some tests that cover the new ValueError exceptions. What do you think?

[MarcoGorelli]

@ricardoV94 nice work!

I think this can be simplified a bit using pytest.mark.paramterize, something like this (I've only written the first two test cases, but it should be easy to extend):

    @pytest.mark.parametrize(
        "mu, n, alpha, expected",
        [
            (5, None, None, "Incompatible parametrization. Must specify either alpha or n."),
            (None, 2, 2, "Incompatible parametrization Can't specify both alpha and n."),
            # other two test cases go here
        ]
    )
    def test_negative_binomial_init_fail(self, mu, n, alpha, expected):
        with Model():
            with pytest.raises(ValueError, match=expected):
                x = NegativeBinomial("x", mu=mu, n=n, alpha=alpha)

[MarcoGorelli]

missing full-stop?

Suggested change

	"Incompatible parametrization Can't specify both alpha and n."
	"Incompatible parametrization. Can't specify both alpha and n."

[ricardoV94]

@MarcoGorelli Thanks for the suggestion. I refactored the code with the pytest.mark.parametrize as you suggested.

I also added a couple of tests for the different permutations of missing/over-specified parameters, do you think it is too much?

[twiecki]

Thanks @ricardoV94! Is this your first PR?

[ricardoV94]

You're welcome. Yes it was the first one for me :)

[twiecki]

Congratulations then! :)

…

On Thu, Oct 1, 2020 at 9:52 PM ricardoV94 ***@***.***> wrote: You're welcome. Yes it was the first one for me :) — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#4134 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFETGC75NLNGNEERMDE4R3SITMYJANCNFSM4R2Z2X6A> .