Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mixture should not allow mixing of discrete and continuous distributions #4511

Closed
ricardoV94 opened this issue Mar 8, 2021 · 4 comments · Fixed by #5629
Closed

Mixture should not allow mixing of discrete and continuous distributions #4511

ricardoV94 opened this issue Mar 8, 2021 · 4 comments · Fixed by #5629
Assignees
Labels

Comments

@ricardoV94
Copy link
Member

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.

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:

The STAN forum has an informative discussion on this:


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.

@ricardoV94
Copy link
Member Author

#3582 raises the same concern (applied to the proposal distributions)

@brandonwillard
Copy link
Contributor

This won't be worth addressing in v3, because v4 will dramatically change the implementation of mixtures, and mixed datatype issues like this can be addressed much more naturally and directly in Aesara.

@larryshamalama
Copy link
Member

We can perhaps substitute all_discrete (which should be removed) by some other similar check in dist. I can create a draft PR on this

@ricardoV94
Copy link
Member Author

ricardoV94 commented Mar 20, 2022

We can perhaps substitute all_discrete (which should be removed) by some other similar check in dist. I can create a draft PR on this

Sure, should be enough to check the dtype of the components is the same in .dist

@ricardoV94 ricardoV94 modified the milestone: v4.0.0b7 Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants