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 DensityDist into v4 #5026

Merged
merged 2 commits into from
Sep 28, 2021
Merged

Conversation

lucianopaz
Copy link
Contributor

@lucianopaz lucianopaz commented Sep 24, 2021

Closes #4831

This PR ports DensityDist into v4. It creates a DensityDistRV class and DensityDist class in a similar way as is done by the Simulator class. When a DensityDist is created, it registers the logp, logcdf and get_moment functions for the associated Op type. The DensityDistRV is used to overload the rng_fn from the base class to be able to run the user supplied random callable with the appropriate signature.

Depending on what your PR does, here are a few things you might want to address in the description:

  • what are the (breaking) changes that this PR makes?

The signature of DensityDist is completely different than in v3:

  • It now takes as positional arguments, the distribution parameters. In v3, these were passed in through a dictionary that was supplied as observed. The associated MultiObservedRV is no longer used or needed and can be removed in a following PR.
  • logp is now an optional keyword only argument instead of the first positional argument. If users try to pass it as the first positional argument, they will get an exception that tells them that they are using the old v3 API and should change.
  • The signature of logp is now different, it takes in the distribution parameters as positional arguments. Something similar happens for random.
  • All the distribution methods: logp, random, logcdf and get_moment are optional. If they are not passed during construction, the RV will be built, but a NotImplementedError will be raised when you try to do something like pm.sample, pm.sample_prior_predictive or get_moment.
  • important background, or details about the implementation
  • are the changes—especially new features—covered by tests and docstrings?

The docstrings were updated and new tests have been added for the different optional methods (except logcdf) which behaves in the same way as logp. If necessary, it's possible to add tests for logcdf too.

@codecov
Copy link

codecov bot commented Sep 25, 2021

Codecov Report

Merging #5026 (b9b9ea1) into main (37ba9a3) will increase coverage by 0.45%.
The diff coverage is 96.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5026      +/-   ##
==========================================
+ Coverage   77.85%   78.31%   +0.45%     
==========================================
  Files         128      129       +1     
  Lines       24349    24400      +51     
==========================================
+ Hits        18958    19108     +150     
+ Misses       5391     5292      -99     
Impacted Files Coverage Δ
pymc/tests/test_model.py 99.11% <ø> (+0.88%) ⬆️
pymc/gp/gp.py 60.25% <50.00%> (+15.33%) ⬆️
pymc/distributions/distribution.py 94.62% <92.15%> (+7.46%) ⬆️
pymc/tests/test_distributions.py 96.34% <100.00%> (+0.33%) ⬆️
pymc/tests/test_distributions_random.py 86.28% <100.00%> (+1.63%) ⬆️
pymc/tests/test_gp.py 86.20% <100.00%> (+1.28%) ⬆️
pymc/tests/test_idata_conversion.py 96.20% <100.00%> (+2.72%) ⬆️
pymc/tests/test_moment.py 100.00% <100.00%> (ø)
pymc/tests/test_parallel_sampling.py 80.64% <100.00%> (+1.65%) ⬆️
pymc/tests/test_sampling.py 97.29% <100.00%> (+0.23%) ⬆️
... and 19 more

RELEASE-NOTES.md Outdated Show resolved Hide resolved
docs/source/Probability_Distributions.rst Outdated Show resolved Hide resolved
docs/source/Probability_Distributions.rst Outdated Show resolved Hide resolved
pymc/distributions/distribution.py Outdated Show resolved Hide resolved
ndim_supp = cls.rv_op.ndim_supp
if not hasattr(output.tag, "test_value"):
size = to_tuple(kwargs.get("size", None)) + (1,) * ndim_supp
output.tag.test_value = np.zeros(size, dtype)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new initval framework will no longer fall back to the test_value. Furthermore this assignment doesn't have a lot of dimensionality flexibility, for example with symbolic size.

My recommendation is to take out the assignment and also the hack below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but the new initial value framework has not been finished, so I’ve used the same hack that Flat and HalfFlat use to get their initial values. I don’t think that this PR should have to wait for the new initial values framework to merge it.
In any case, over the new framework is in place, we could change the default initialisation used by density dist.

pymc/tests/test_moment.py Outdated Show resolved Hide resolved
@michaelosthege michaelosthege added this to the vNext (4.0.0) milestone Sep 27, 2021
Copy link
Member

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it 👍

@twiecki twiecki merged commit 1048b69 into pymc-devs:main Sep 28, 2021
@twiecki
Copy link
Member

twiecki commented Sep 28, 2021

Thanks @lucianopaz!

@ricardoV94 ricardoV94 mentioned this pull request Sep 28, 2021
26 tasks
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 this pull request may close these issues.

Refactor or deprecate DensityDist
3 participants