Refactored Bound for v4 #4815

kc611 · 2021-06-28T10:52:02Z

This PR refactors the Bound distribution for v4

pymc3/distributions/bound.py

twiecki · 2021-07-05T15:59:14Z

@kc611 How close do you think this is to merge? If there's still a ways to go, we might want to focus on the aeppl integration instead where we can get mixture almost for free. CC @brandonwillard

kc611 · 2021-07-05T17:48:49Z

I'd say we're halfway through over here. But if we're bringing in aeppl aesara-devs/aeppl#22 PR of CensoredRV can be a much better replacement for Bound logic.

cc @ricardoV94

ricardoV94 · 2021-07-05T18:11:57Z

CensoredRV is not a good replacement. We would need something like TruncatedRV but that is still some time away

codecov · 2021-07-07T13:28:22Z

Codecov Report

Merging #4815 (9e6631d) into main (6a75744) will increase coverage by 0.43%.
The diff coverage is 100.00%.

❗ Current head 9e6631d differs from pull request most recent head d105682. Consider uploading reports for the commit d105682 to get more accurate results

@@            Coverage Diff             @@
##             main    #4815      +/-   ##
==========================================
+ Coverage   73.17%   73.61%   +0.43%     
==========================================
  Files          86       86              
  Lines       13868    13840      -28     
==========================================
+ Hits        10148    10188      +40     
+ Misses       3720     3652      -68

Impacted Files	Coverage Δ
pymc3/distributions/bound.py	`100.00% <100.00%> (+67.88%)`	⬆️
pymc3/model.py	`83.47% <100.00%> (-0.12%)`	⬇️
pymc3/backends/report.py	`89.51% <0.00%> (-2.10%)`	⬇️
pymc3/distributions/distribution.py	`80.43% <0.00%> (-1.45%)`	⬇️
pymc3/parallel_sampling.py	`87.20% <0.00%> (-1.02%)`	⬇️
pymc3/util.py	`73.50% <0.00%> (-0.85%)`	⬇️
pymc3/distributions/continuous.py	`96.26% <0.00%> (+0.01%)`	⬆️
pymc3/data.py	`81.77% <0.00%> (+0.44%)`	⬆️

pymc3/distributions/bound.py

pymc3/tests/test_distributions.py

pymc3/distributions/bound.py

ricardoV94 · 2021-07-22T11:58:38Z

pymc3/distributions/bound.py

-            lower = None
-        if isinstance(upper, Real) and upper == np.inf:
-            upper = None
+    def __new__(cls, name, distribution, lower=None, upper=None, size=None, **kwargs):


If we are in a modelcontext we should check here that the distribution is not in model (which would mean the user did not use .dist() and the original variable would be accounted on the model logp as well.

~~In addition we can add the check that it is a TensorVariable, if not, the user is probably trying to work with the old API~~ I see you already did this. Then the first error message can also go there

twiecki · 2021-07-22T21:10:39Z

pymc3/distributions/bound.py

@@ -232,32 +133,70 @@ class Bound:
    .. code-block:: python

        with pm.Model():
-            NegativeNormal = pm.Bound(pm.Normal, upper=0.0)


Do we not support this use-case anymore?

No, you have to pass a .dist(). There is a very informative error message with a code example if users do that.

This was done to simplify the implementation and also be more in line with the rest of the API (e.g when you specify distributions for Mixtures, LKJCorr priors, RWs?). Also in the future, Truncated and Censored distributions would work the same way.

~~I checked the original PRs for Bound and this was actually the initial intended API, but they couldn't make it work.~~ It was not, but it came up a couple of times, e.g: #2277 (comment)

pymc3/distributions/bound.py

pymc3/tests/test_distributions.py

pymc3/distributions/bound.py

ricardoV94

Left some comments, We should wrap the lower / upper with at.as_tensor_variable([floatX|intX]), probably in the .dist method of the private classes _[Discrete|Continuous]Bounded, to not interfere with the cls.set_values.

Also after #4867 that logic can probably be simplified, as we will be able to just return symbolic initvals on demand

twiecki · 2021-08-13T15:09:19Z

Thanks @kc611 and @ricardoV94!

kc611 commented Jun 28, 2021

View reviewed changes