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

Expand DensityDist docstrings #5641

Closed
ricardoV94 opened this issue Mar 22, 2022 · 4 comments
Closed

Expand DensityDist docstrings #5641

ricardoV94 opened this issue Mar 22, 2022 · 4 comments

Comments

@ricardoV94
Copy link
Member

ricardoV94 commented Mar 22, 2022

We should include code snippets about how to use the DensityDist, as 1) the API has changed since V3, and 2) it has always been a source of confusion

class DensityDist(NoDistribution):
"""A distribution that can be used to wrap black-box log density functions.
Creates a Distribution and registers the supplied log density function to be used
for inference. It is also possible to supply a `random` method in order to be able
to sample from the prior or posterior predictive distributions.
"""

We should also add a warning, similar to the Interval transform, stating the logp / logcdf / random / get_moment functions cannot rely on nonlocal variables, but only on constants + distribution parameters:

.. warning:: Expressions returned by `bounds_fn` should depend only on the
distribution inputs or other constants. Expressions that depend on other
symbolic variables, including nonlocal variables defined in the model
context will likely break sampling.

See #5614 and related issue for more context on this limitation

Bonus points for including an example of a Multivariate distribution and explaining ndim_supp and ndim_params

Not everything has to be done at once or by the same person!

@OriolAbril
Copy link
Member

Also, the __new__ method should have no docstring. All the content there should be in the class docstring, otherwise it doesn't appear in the documentation

@ricardoV94
Copy link
Member Author

Also, the __new__ method should have no docstring. All the content there should be in the class docstring, otherwise it doesn't appear in the documentation

Created a specific issue for that: #6031

@ricardoV94
Copy link
Member Author

There have been a couple of PRs improving the status. I think this can be closed

https://docs.pymc.io/en/latest/api/distributions/generated/pymc.CustomDist.html

@OriolAbril
Copy link
Member

closing then, it still needs to be fixed to comply with #5459 but there is already that other issue open for this.

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

No branches or pull requests

2 participants