Add flag to disable bounds check for speed-up #4377
Conversation
There was a problem hiding this comment.
I'd be interested in seeing an example where this can (should?) be enabled, and one where it shouldn't. Sounds too good to be true!
For instance, it is not clear to me what to do with
with pm.Model():
sd = pm.HalfNormal('sd', 1)
obs = pm.Normal('obs', 0, sd)I would guess that the automatic log-transform of sd would mean that I could disable bounds checking?
|
@ColCarroll Yes, you can disable that for basically any well-specified model. |
pymc3/distributions/dist_math.py
Outdated
| @@ -67,6 +68,12 @@ 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")) | |||
There was a problem hiding this comment.
Im more curious than opposed, but why not just skip the bound check where its called instead of no opping in the check bounds function?
There was a problem hiding this comment.
In that case we would need to add an if-clause to every Distribution.
There was a problem hiding this comment.
ugh yea that would suck. This is better
Codecov Report
@@ Coverage Diff @@
## master #4377 +/- ##
=======================================
Coverage 87.95% 87.96%
=======================================
Files 88 88
Lines 14493 14501 +8
=======================================
+ Hits 12748 12756 +8
Misses 1745 1745
|
|
OK, this is ready for review. |
|
I can review later today if no one else gets to it -- any chance you can screenshot a usage example, showing the speedup (i'd imagine you fit the same model twice, and just set the flag to |
|
|
|
Looks great! I guess there should be general encouragement to enable this for a few weeks, then maybe we think about enabling by default? |
|
Hmmm, thinking I should have just called this |
|
Or |
|
Oh, yeah. |
* Add flag to disable bounds check. * Move check for model and flag into single line as per @colcarrolls's suggestion. * modelcontext raises a TypeError if no model is found. Catch that. * Add comment. * Add mention in release-notes. * Add test for when boundaries are disabled.
Closes #4376.
Depending on what your PR does, here are a few things you might want to address in the description:
None.
Disabling bounds checks gave quite a speed-up (66%) in my experiments.