Repository: pymc-devs/pymc

Issue:
Mixture should not allow mixing of discrete and continuous distributions
Right now the Mixture class allows to mix distributions of any kind, leading to improper logp evaluations where probability densities are mixed with probability masses.

```python
norm = pm.Normal.dist(0, 1)
pois = pm.Poisson.dist(1)
mix = pm.Mixture.dist([0.5, 0.5], comp_dists=[norm, pois], shape=1)

# Observed integer could only have come from Poisson component, so logp should be:
np.log(0.5 * np.exp(pois.logp(0))).eval()  # -1.69

# But it's higher:
mix.logp(0).eval()  # -0.958

# Observed float could only have come from Normal component, so logp should be:
np.log(0.5 * np.exp(norm.logp(1/3))).eval() # -1.66

# But it's higher:
mix.logp(1/3).eval()  # -0.93

# This is not a problem when evaluating points where the domains do not overlap:
mix.logp(-1).eval()  # -2.11
np.log(0.5 * np.exp(norm.logp(-1))).eval()  # -2.11
```

Questions related to the mixture of continuous and discrete distributions crop up now and then on the Discourse:
* https://discourse.pymc.io/t/sampling-from-a-learned-mixture-of-zeros-and-lognormal/3671
* https://discourse.pymc.io/t/zero-inflated-student-t/6170
* https://discourse.pymc.io/t/zero-inflated-normal/6857
* https://discourse.pymc.io/t/mixture-of-continuous-and-discrete-logp/2392

The STAN forum has an informative discussion on this:
* https://discourse.mc-stan.org/t/zero-inflated-models-in-stan/19496

***

It seems to me like this is an area where we could easily nudge users in the right direction, by raising an informative value error. 

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/pymc-devs/pymc/pull/5629?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs) Report
> Merging [#5629](https://codecov.io/gh/pymc-devs/pymc/pull/5629?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs) (12bce28) into [main](https://codecov.io/gh/pymc-devs/pymc/commit/849a64d7c580e201ea027ac473fa906d9dd9b682?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs) (849a64d) will **decrease** coverage by `42.89%`.
> The diff coverage is `22.22%`.

[![Impacted file tree graph](https://codecov.io/gh/pymc-devs/pymc/pull/5629/graphs/tree.svg?width=650&height=150&src=pr&token=JFuXtOJ4Cb&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs)](https://codecov.io/gh/pymc-devs/pymc/pull/5629?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs)

```diff
@@             Coverage Diff             @@
##             main    #5629       +/-   ##
===========================================
- Coverage   87.67%   44.77%   -42.90%     
===========================================
  Files          76       76               
  Lines       13717    13716        -1     
===========================================
- Hits        12026     6141     -5885     
- Misses       1691     7575     +5884     
```


| [Impacted Files](https://codecov.io/gh/pymc-devs/pymc/pull/5629?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs) | Coverage Δ | |
|---|---|---|
| [pymc/distributions/mixture.py](https://codecov.io/gh/pymc-devs/pymc/pull/5629/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9kaXN0cmlidXRpb25zL21peHR1cmUucHk=) | `27.42% <22.22%> (-67.31%)` | :arrow_down: |
| [pymc/ode/utils.py](https://codecov.io/gh/pymc-devs/pymc/pull/5629/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9vZGUvdXRpbHMucHk=) | `15.62% <0.00%> (-84.38%)` | :arrow_down: |
| [pymc/step\_methods/mlda.py](https://codecov.io/gh/pymc-devs/pymc/pull/5629/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9zdGVwX21ldGhvZHMvbWxkYS5weQ==) | `12.53% <0.00%> (-83.85%)` | :arrow_down: |
| [pymc/variational/updates.py](https://codecov.io/gh/pymc-devs/pymc/pull/5629/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy92YXJpYXRpb25hbC91cGRhdGVzLnB5) | `11.33% <0.00%> (-80.79%)` | :arrow_down: |
| [pymc/smc/smc.py](https://codecov.io/gh/pymc-devs/pymc/pull/5629/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9zbWMvc21jLnB5) | `18.62% <0.00%> (-79.37%)` | :arrow_down: |
| [pymc/gp/gp.py](https://codecov.io/gh/pymc-devs/pymc/pull/5629/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9ncC9ncC5weQ==) | `18.68% <0.00%> (-74.50%)` | :arrow_down: |
| [pymc/tuning/starting.py](https://codecov.io/gh/pymc-devs/pymc/pull/5629/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy90dW5pbmcvc3RhcnRpbmcucHk=) | `19.51% <0.00%> (-73.18%)` | :arrow_down: |
| [pymc/model\_graph.py](https://codecov.io/gh/pymc-devs/pymc/pull/5629/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9tb2RlbF9ncmFwaC5weQ==) | `15.86% <0.00%> (-69.66%)` | :arrow_down: |
| [pymc/step\_methods/slicer.py](https://codecov.io/gh/pymc-devs/pymc/pull/5629/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9zdGVwX21ldGhvZHMvc2xpY2VyLnB5) | `26.76% <0.00%> (-69.02%)` | :arrow_down: |
| [pymc/gp/cov.py](https://codecov.io/gh/pymc-devs/pymc/pull/5629/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9ncC9jb3YucHk=) | `29.20% <0.00%> (-68.88%)` | :arrow_down: |
| ... and [50 more](https://codecov.io/gh/pymc-devs/pymc/pull/5629/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs) | |

ricardoV94: > Is there a test that should be added?

Always!
larryshamalama: > > Is there a test that should be added?
> 
> Always!

Can you share a quick pointer on how to catch errors?
ricardoV94: > > > Is there a test that should be added?
> > 
> > 
> > Always!
> 
> Can you share a quick pointer on how to catch errors?

Usually with `with pytest.raises(ExpectedError, match="expected error string"): ...`

There are a bunch of examples in the codebase, here is just a random one: https://github.com/pymc-devs/pymc/blob/8b063f9c3b344753acdf91416cf2bb3c4533f80a/pymc/tests/test_aesaraf.py#L60-L61


Patch:
diff --git a/pymc/distributions/mixture.py b/pymc/distributions/mixture.py
index 16e356160..0b93a0c57 100644
--- a/pymc/distributions/mixture.py
+++ b/pymc/distributions/mixture.py
@@ -29,32 +29,16 @@ from pymc.aesaraf import change_rv_size
 from pymc.distributions import transforms
 from pymc.distributions.continuous import Normal, get_tau_sigma
 from pymc.distributions.dist_math import check_parameters
-from pymc.distributions.distribution import (
-    Discrete,
-    Distribution,
-    SymbolicDistribution,
-    _moment,
-    moment,
-)
+from pymc.distributions.distribution import SymbolicDistribution, _moment, moment
 from pymc.distributions.logprob import logcdf, logp
 from pymc.distributions.shape_utils import to_tuple
 from pymc.distributions.transforms import _default_transform
 from pymc.util import check_dist_not_registered
-from pymc.vartypes import discrete_types
+from pymc.vartypes import continuous_types, discrete_types
 
 __all__ = ["Mixture", "NormalMixture"]
 
 
-def all_discrete(comp_dists):
-    """
-    Determine if all distributions in comp_dists are discrete
-    """
-    if isinstance(comp_dists, Distribution):
-        return isinstance(comp_dists, Discrete)
-    else:
-        return all(isinstance(comp_dist, Discrete) for comp_dist in comp_dists)
-
-
 class MarginalMixtureRV(OpFromGraph):
     """A placeholder used to specify a log-likelihood for a mixture sub-graph."""
 
@@ -182,6 +166,19 @@ class Mixture(SymbolicDistribution):
                 UserWarning,
             )
 
+        if len(comp_dists) > 1:
+            all_continuous = all(comp_dist.dtype in continuous_types for comp_dist in comp_dists)
+            all_discrete = all(comp_dist.dtype in discrete_types for comp_dist in comp_dists)
+
+            if not (
+                all(comp_dist.dtype in continuous_types for comp_dist in comp_dists)
+                or all(comp_dist.dtype in discrete_types for comp_dist in comp_dists)
+            ):
+                raise ValueError(
+                    "All distributions in comp_dists must be either discrete or continuous.\n"
+                    "See the following issue for more information: https://github.com/pymc-devs/pymc/issues/4511."
+                )
+
         # Check that components are not associated with a registered variable in the model
         components_ndim_supp = set()
         for dist in comp_dists:


Test patch:
diff --git a/pymc/tests/test_mixture.py b/pymc/tests/test_mixture.py
index 13898e651..2cab2151a 100644
--- a/pymc/tests/test_mixture.py
+++ b/pymc/tests/test_mixture.py
@@ -721,6 +721,21 @@ class TestMixture(SeededTest):
             assert isinstance(comp_dist.owner.op, RandomVariable)
             assert tuple(comp_dist.shape.eval()) == expected_shape
 
+    def test_preventing_mixing_cont_and_discrete(self):
+        with pytest.raises(
+            ValueError,
+            match="All distributions in comp_dists must be either discrete or continuous.",
+        ):
+            with Model() as model:
+                mix = Mixture(
+                    "x",
+                    w=[0.5, 0.3, 0.2],
+                    comp_dists=[
+                        Categorical.dist(np.tile(1 / 3, 3)),
+                        Normal.dist(np.ones(3), 3),
+                    ],
+                )
+
 
 class TestNormalMixture(SeededTest):
     def test_normal_mixture_sampling(self):