Repository: pymc-devs/pymc
Pull request: Add flag to disable bounds check for speed-up
Closes #4376.

Depending on what your PR does, here are a few things you might want to address in the description:
+ [x] what are the (breaking) changes that this PR makes?
None.
+ [x] important background, or details about the implementation
Disabling bounds checks gave quite a speed-up (66%) in my experiments. 
+ [x] are the changes—especially new features—covered by tests and docstrings?
+ [ ] consider adding/updating relevant example notebooks
+ [x] right before it's ready to merge, mention the PR in the RELEASE-NOTES.md


Problem statement:
66% speed-up by removing bound checks
I added a single line of code and got a 66% speed-up. Most of our distributions have various bound checks. `pm.Normal.logp` for example checks that `sigma > 0` *for every logp eval*.

These checks are useful when building a new model, for example, if I put a `pm.Normal` on my sigma and it proposes negative values I want the logp to become `-inf`.

However, once I have built the model and know that all my parameters can only take on valid ranges I don't need these checks anymore.

So just by making `bound()` return `logp` directly (i.e turning it into a noop) I got a nice 66% speed-up on the stochastic vol model.

My question is how do we best allow the user to configure this.

One idea is `with pm.Model(disable_bounds_check=True)`.

Hints from issue discussion:

Commit message: Add flag to disable bounds check.

Patch to review:
diff --git a/pymc3/distributions/dist_math.py b/pymc3/distributions/dist_math.py
index e302000735..4e6d639d1a 100644
--- a/pymc3/distributions/dist_math.py
+++ b/pymc3/distributions/dist_math.py
@@ -33,6 +33,7 @@
 
 from pymc3.distributions.shape_utils import to_tuple
 from pymc3.distributions.special import gammaln
+from pymc3.model import modelcontext
 from pymc3.theanof import floatX
 
 f = floatX
@@ -67,6 +68,13 @@ def bound(logp, *conditions, **kwargs):
     -------
     logp with elements set to -inf where any condition is False
     """
+
+    # If called inside a model context, see if bounds check is disabled
+    model = modelcontext(kwargs.get("model"))
+    if model is not None:
+        if model.disable_bounds_check:
+            return logp
+
     broadcast_conditions = kwargs.get("broadcast_conditions", True)
 
     if broadcast_conditions:
diff --git a/pymc3/model.py b/pymc3/model.py
index 14d7244278..6a6e159b6c 100644
--- a/pymc3/model.py
+++ b/pymc3/model.py
@@ -809,6 +809,11 @@ class Model(Factor, WithMemoization, metaclass=ContextMeta):
         temporarily in the model context. See the documentation
         of theano for a complete list. Set config key
         ``compute_test_value`` to `raise` if it is None.
+    disable_bounds_check: bool
+        Disable checks that ensure that input parameters to distributions
+        are in a valid range. If your model is built in a way where you
+        know your parameters can only take on valid values you can disable
+        this for increased speed.
 
     Examples
     --------
@@ -895,11 +900,14 @@ def __new__(cls, *args, **kwargs):
         instance._theano_config = theano_config
         return instance
 
-    def __init__(self, name="", model=None, theano_config=None, coords=None):
+    def __init__(
+        self, name="", model=None, theano_config=None, coords=None, disable_bounds_check=False
+    ):
         self.name = name
         self.coords = {}
         self.RV_dims = {}
         self.add_coords(coords)
+        self.disable_bounds_check = disable_bounds_check
 
         if self.parent is not None:
             self.named_vars = treedict(parent=self.parent.named_vars)


Code review comments:
diff_hunk: @@ -67,6 +68,13 @@ def bound(logp, *conditions, **kwargs):
     -------
     logp with elements set to -inf where any condition is False
     """
+
+    # If called inside a model context, see if bounds check is disabled
+    model = modelcontext(kwargs.get("model"))
+    if model is not None:
path: pymc3/distributions/dist_math.py
text: @user1:
You can use short-circuiting behavior here safely: 
```suggestion
    if model is not None and model.disable_bounds_check:
```