Add DensityDist moment

[lucianopaz]

Adds a default moment of 0 for the DensityDist distribution. This allows the DensityDist to be used by pm.sample even if the users don't explicitly pass a get_moment function to it.

This PR contributes to #5078

• pymc.distributions.distribution.DensityDist

[ricardoV94]

This won't work for multivariate distributions...

Should we raise a NotImplementedError in those cases. We can inspect the RV to know the number of dimensions.

Also we should port these tests to the present module: https://github.com/pymc-devs/pymc/blob/main/pymc/tests/test_moment.py ?

[codecov]

Codecov Report

Merging #5159 (45e33bc) into main (c0c5a80) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5159      +/-   ##
==========================================
+ Coverage   78.10%   78.11%   +0.01%     
==========================================
  Files          88       88              
  Lines       14161    14168       +7     
==========================================
+ Hits        11060    11067       +7     
  Misses       3101     3101              

Impacted Files	Coverage Δ
pymc/distributions/distribution.py	94.70% <100.00%> (+0.20%)	⬆️

[lucianopaz]

This won't work for multivariate distributions...

Should we raise a NotImplementedError in those cases. We can inspect the RV to know the number of dimensions.

I hadn’t thought of that. How can we know the shape of the support dimensions? We could potentially return a tensor with the support dimensions as broadcastable. Would that then be broadcasted correctly at the later sampling stage?

Also we should port these tests to the present module: https://github.com/pymc-devs/pymc/blob/main/pymc/tests/test_moment.py ?

Yes, and maybe also remove that module entirely

[lucianopaz]

By the way, this should be very similar to what we’ll want to do for the Simulator distribution, so once we agree on how we handle multivariate distributions, I can port it to the simulator

[ricardoV94]

I hadn’t thought of that. How can we know the shape of the support dimensions? We could potentially return a tensor with the support dimensions as broadcastable. Would that then be broadcasted correctly at the later sampling stage?

We can access ndim_supp and ndims_params class attributes from the RV Op, but that only gives the minimum dimensions, and we have no way of knowing how they map to each other.

We can also just return something like at.zeros_like(rv) which should work in case the user implemented the random function. But in that case defaulting to a sample from the prior might actually be more sensible.

Yes, and maybe also remove that module entirely

Sounds good.

[ricardoV94]

By the way, this should be very similar to what we’ll want to do for the Simulator distribution, so once we agree on how we handle multivariate distributions, I can port it to the simulator

Yeah I think so too. My only question is whether for the Simulator a draw from the random function may actually be the preferred starting point. We haven't really explored how well Simulators behave in non SMC samplers... In SMC it doesn't matter because we always start sampling form the prior and not from moments.

@aloctavodia any opinion on this?

[lucianopaz]

@ricardoV94, I wrote a very small fallback option that uses the size of a random draw to inform the size of the moment, in the case of multivariate DensityDist. If the users haven't supplied the random method, then a TypeError is raised. Let me know what you think about this solution. If you think it's ok, I'll rebase the PR.

[ricardoV94]

@ricardoV94, I wrote a very small fallback option that uses the size of a random draw to inform the size of the moment, in the case of multivariate DensityDist. If the users haven't supplied the random method, then a TypeError is raised. Let me know what you think about this solution. If you think it's ok, I'll rebase the PR.

Sounds good. I'll check later more carefully for any bugs/issues but you can rebase in the meantime.

[ricardoV94]

Looks great @lucianopaz, just need to resolve the conflicts with the main branch

[lucianopaz]

Looks great @lucianopaz, just need to resolve the conflicts with the main branch

Damn, I can't keep up with all the merged PRs

[ricardoV94]

Thanks @lucianopaz!