WIP Implement ZeroInflatedPoisson as a subclass of Mixture

[AustinRochford]

Putting this up for feedback, as it is not quite as straightforward as I would have liked.

Due to #1449 and its fix #1452, we can't use ConstantDist for the zero component, since it no longer broadcasts the way we need it to in this instance.

More specifically, since #1452, if any of the elements of pm.ConstantDist.dist(0).logp(value) are non-zero, the result will be -np.inf. The Zero hack included below restores the pre-#1452 broadcasting behavior by bypassing bound and using tt.switch directly. Using this hack, pm.Zero.dist().logp(value) will be an array whose entries are zero when the corresponding entry in value is zero and -np.inf when the corresponding entry in value is non-zero. The use of logsumexp in Mixture will gracefully handle these -np.infs, as desired.

My question for other is if we want to use this Zero hack to implement ZeroInflated* distributions as subclasses of Mixture or leave them as-is. Obviously we do not need to export Zero to the global namespace.

If we decide to go forward with this approach, I will port ZeroInflatedNegativeBinomial as well and fix any test that this might break.

[AustinRochford]

An alternative would be to change the bound in ConstantDist's logp to use tt.switch.

[twiecki]

This would make ZeroInflatedPoisson https://github.com/pymc-devs/pymc3/blob/master/pymc3/distributions/discrete.py#L445 and, potentially, ZeroInflatedNegativeBinomial https://github.com/pymc-devs/pymc3/blob/master/pymc3/distributions/discrete.py#L495 obsolete, correct?

[twiecki]

An alternative would be to change the bound in ConstantDist's logp to use tt.switch.

That doesn't sound unreasonable. I suppose there's no way to get the broadcasting in bound back without reintroducing the earlier problem?

[AustinRochford]

Yes, I would remove those other implementations.

Agreed that changing ConstantDist should be fine. Searching the code, it doesn't seem to be used in very many places

I have not been able to think of a way to get bound to automatically broadcast correctly in every situation, but that would be ideal.

[twiecki]

Agreed that changing ConstantDist should be fine. Searching the code, it doesn't seem to be used in very many places

That's probably best then, both are hacks in a way, just one with less code. Adding a comment on why we're using switch here should help then also.

[springcoil]

Seems the broadcasting is merged now @AustinRochford can you check if this works with the new code that @fonnesbeck submitted recently.

[twiecki]

@AustinRochford Any progress here? Seems like the original zero-inflated-poisson is wrong.

[AustinRochford]

@twiecki nothing recently; there are a few things left to do. Unfortunately we have entered our busy holiday season at work, so it may take a while for me to get this into shape.

[twiecki]

ok, no worries obviously and thanks for the update.