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

Refactor or deprecate DensityDist #4831

Closed
ricardoV94 opened this issue Jul 2, 2021 · 2 comments · Fixed by #5026
Closed

Refactor or deprecate DensityDist #4831

ricardoV94 opened this issue Jul 2, 2021 · 2 comments · Fixed by #5026

Comments

@ricardoV94
Copy link
Member

ricardoV94 commented Jul 2, 2021

We need to decide whether we want to deprecate DensityDist and if so, provide at least documentation on how to add simple logp terms to a model or how to quickly implement a custom Distribution.

If we want to refactor it, it is probably good time to get rid of the whole dictionary as observed thing and force the distribution parameters to go into the signature as normal arguments in other distributions

# V3 allows this
def my_logp(value, param1, param2):
    return value * param1 - param2

param1 = pm.Normal('param1', 0, 1)
param2 = pm.Normal('param2', 0, 1)
y = pm.DensityDist('y', my_logp, observed=dict(value=data, param1=param1, param2=param2))
# V4 should work only like this (if not deprecated)
y = pm.DensityDist('y', my_logp, param1, param2, obeserved=data)
@ricardoV94
Copy link
Member Author

CC @lucianopaz, if you want to assign this to yourself. Seems like we are pretty close to having a working solution

@OriolAbril
Copy link
Member

OriolAbril commented Aug 23, 2021

looks related to #4534, and I also wanted to leave a link to https://discourse.pymc.io/t/using-a-random-variable-as-observed/7184/5 so we are aware of all the (in my opinion strange and redundant) things DensityDist allows, expecially when taking a dictionary as observed (which I also think is the only Distribution that does).

As I have commented a few times and started changing in #4433, I think it's a bad idea to allow FreeRVs to be values in the dictionary passed as observed. If this is a feature that is really important to DensityDist, I would suggest doing this givens extra argument or following @ricardoV94 suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants