changed default values for TruncatedNormal lower and upper from None to -np.inf…

[gilliss]

Addressing issue #3248.

Changed default values for pm.distributions.continuous.TruncatedNormal lower and upper from None and None to -np.inf and np.inf.
This avoids ValueError: Unexpected type in draw_value: <class 'NoneType'>, which had appeared upon calls of

pm.TruncatedNormal.dist(lower = 0.).random() # error
pm.TruncatedNormal.dist(upper = 100.).random() # error
pm.TruncatedNormal.dist().random() # error

[twiecki]

@gilliss Thanks, can you add a test?

[gilliss]

@twiecki, I'm not familiar with how the tests/ directory works, but I've made a guess.
Working from
https://github.com/pymc-devs/pymc3/blob/master/pymc3/tests/test_distributions_random.py#L220,
perhaps I could add two new functions to BaseTestCases.BaseTestCase, as summarized at the link below?
gilliss@51fd1fb#diff-b04ab82851a28b3674b2d08eb3c4d5f9

[twiecki]

@gilliss Apparently we didn't test sampling from the TruncatedNormal if either bound was not supplied. Such a test should break before this PR but pass now. Thus I would add a test specifically for that case.

[gilliss]

@twiecki, I've updated the PR with a specific test tacked onto the bottom of test_distributions_random.py.
https://github.com/pymc-devs/pymc3/pull/3250/files
The proposed test passes when I run pytest test_distributions_random.py in my local environment.

[twiecki]

Best to follow the existing logic, so you could just duplicate https://github.com/pymc-devs/pymc3/blob/893414fdbada86a9741553563ab725028864e319/pymc3/tests/test_distributions_random.py#L220 and remove one of the two bounds and then add another method here: https://github.com/pymc-devs/pymc3/blob/893414fdbada86a9741553563ab725028864e319/pymc3/tests/test_distributions_random.py#L429

[gilliss]

Thanks for your help, @twiecki.

• I added class TestTruncatedNormalLower(BaseTestCases.BaseTestCase) to test TruncatedNormal when no upper bound is set.
• I did not add a test within class TestScalarParameterSamples(SeededTest) because those tests call pymc3_random which seems to forcibly set distribution parameters (including infs) based on the paramdomains arg. It seemed like the test_truncated_normal might already do the trick.
• In class TruncatedNormal(BoundedContinuous), I updated the logic used to set self._defaultval.

[twiecki]

@gilliss Pretty good motivation for why tests are a good idea :). Travis seems unhappy but I don't think it's related to this.

[gilliss]

@twiecki, another pass.
The tests on TestTruncatedNormalUpper revealed that the infs were messing with TruncatedNormal's normalization method. So, I changed TruncatedNormal._normalization to check for infs instead of Nones-- I hope I didn't overstep my understanding of the code/statistics.
TruncatedNormal.logp also checks for Nones, but I think that may be OK there.

[twiecki]

I think we need to fix these too: https://github.com/pymc-devs/pymc3/pull/3250/files#diff-1837763aea633f0d6324583c00051d83R647

Not sure why the tests wouldn't have caught that, can you look into what the current test actually tests, if not the logp?

[gilliss]

It's the assert_almost_equal that fails in
pymc3/blob/master/pymc3/tests/test_distributions.py#L440.

�[1m>           assert_almost_equal(logp(pt), logp_reference(pt), decimal=decimal, err_msg=str(pt))�[0m
�[1m�[31mE           AssertionError: �[0m
�[1m�[31mE           Arrays are not almost equal to 6 decimals�[0m
�[1m�[31mE           {'mu': array(2.1), 'sd': array(0.99), 'lower': array(-1.5), 'upper': array(0.9), 'value': array(-1.)}�[0m
�[1m�[31mE           (mismatch 100.0%)�[0m
�[1m�[31mE            x: array(-5.811449)�[0m
�[1m�[31mE            y: array(-3.627489)�[0m

�[1m�[31mpymc3/tests/test_distributions.py�[0m:448: AssertionError

It looks like this particular test had set the lower and upper params, so it's not a default params issue. TruncatedNormal.logp uses Normal.logp, which itself passes tests, so the issue is likely the additional bounds/conditions imposed in TruncatedNormal.logp.

I'll try changing the None checks to inf checks in TruncatedNormal.logp.

[twiecki]

It looks like this particular test had set the lower and upper params, so it's not a default params issue.

I think we might be setting limits for the scipy distribution, but not the other one.

Also, can you rebase from master?

[twiecki]

I think your current one just tests the overall behavior of the RV (shape etc). More critical would be a test identical to https://github.com/pymc-devs/pymc3/blob/e9d892ceda642cb09c7772bc776c5388a0ce370d/pymc3/tests/test_distributions_random.py#L437.

[gilliss]

It seems like a variety of upper and lower settings are already tested by
https://github.com/pymc-devs/pymc3/blob/e9d892ceda642cb09c7772bc776c5388a0ce370d/pymc3/tests/test_distributions_random.py#L437
Are you thinking something like

def test_truncated_normal_lower(self):
        def ref_rand(size, mu, sd, lower, upper):
            return st.truncnorm.rvs((lower-mu)/sd, (np.inf-mu)/sd, size=size, loc=mu, scale=sd)
        pymc3_random(pm.TruncatedNormal, {'mu': R, 'sd': Rplusbig, 'lower':-Rplusbig},
                     ref_rand=ref_rand)

where the upper has been forced to np.inf?

[twiecki]

Yes, exactly. You can probably just set np.inf there though :).

[twiecki]

@gilliss Are you still working on this? Seems like there are test-failures:

=================================== FAILURES ===================================
____________ TestScalarParameterSamples.test_truncated_normal_lower ____________
self = <pymc3.tests.test_distributions_random.TestScalarParameterSamples object at 0x7f45a249fe10>
    def test_truncated_normal_lower(self):
        def ref_rand(size, mu, sigma, lower, upper):
            return st.truncnorm.rvs((lower-mu)/sigma, np.inf, size=size, loc=mu, scale=sigma)
        pymc3_random(pm.TruncatedNormal, {'mu': R, 'sigma': Rplusbig, 'lower':-Rplusbig},
>                    ref_rand=ref_rand)
pymc3/tests/test_distributions_random.py:485: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
dist = <class 'pymc3.distributions.continuous.TruncatedNormal'>
paramdomains = {'lower': <pymc3.tests.test_distributions.Domain object at 0x7f45a22f5ac8>, 'mu': <pymc3.tests.test_distributions.Domain object at 0x7f4612b21160>, 'sigma': <pymc3.tests.test_distributions.Domain object at 0x7f4612bbb5f8>}
ref_rand = <function TestScalarParameterSamples.test_truncated_normal_lower.<locals>.ref_rand at 0x7f45a3748840>
valuedomain = <pymc3.tests.test_distributions.Domain object at 0x7f4612b216d8>
size = 10000, alpha = 0.05, fails = 10, extra_args = None, model_args = {}
    def pymc3_random(dist, paramdomains, ref_rand, valuedomain=Domain([0]),
                     size=10000, alpha=0.05, fails=10, extra_args=None,
                     model_args=None):
        if model_args is None:
            model_args = {}
        model = build_model(dist, valuedomain, paramdomains, extra_args)
        domains = paramdomains.copy()
        for pt in product(domains, n_samples=100):
            pt = pm.Point(pt, model=model)
            pt.update(model_args)
            p = alpha
            # Allow KS test to fail (i.e., the samples be different)
            # a certain number of times. Crude, but necessary.
            f = fails
            while p <= alpha and f > 0:
                s0 = model.named_vars['value'].random(size=size, point=pt)
>               s1 = ref_rand(size=size, **pt)
E               TypeError: ref_rand() missing 1 required positional argument: 'upper'
pymc3/tests/test_distributions_random.py:38: TypeError
____________ TestScalarParameterSamples.test_truncated_normal_upper ____________
self = <pymc3.tests.test_distributions_random.TestScalarParameterSamples object at 0x7f45a22f5c88>
    def test_truncated_normal_upper(self):
        def ref_rand(size, mu, sigma, lower, upper):
            return st.truncnorm.rvs(-np.inf, (upper-mu)/sigma, size=size, loc=mu, scale=sigma)
        pymc3_random(pm.TruncatedNormal, {'mu': R, 'sigma': Rplusbig, 'upper':Rplusbig},
>                    ref_rand=ref_rand)
pymc3/tests/test_distributions_random.py:491: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
dist = <class 'pymc3.distributions.continuous.TruncatedNormal'>
paramdomains = {'mu': <pymc3.tests.test_distributions.Domain object at 0x7f4612b21160>, 'sigma': <pymc3.tests.test_distributions.Domain object at 0x7f4612bbb5f8>, 'upper': <pymc3.tests.test_distributions.Domain object at 0x7f4612bbb5f8>}
ref_rand = <function TestScalarParameterSamples.test_truncated_normal_upper.<locals>.ref_rand at 0x7f45a20fcc80>
valuedomain = <pymc3.tests.test_distributions.Domain object at 0x7f4612b216d8>
size = 10000, alpha = 0.05, fails = 10, extra_args = None, model_args = {}
    def pymc3_random(dist, paramdomains, ref_rand, valuedomain=Domain([0]),
                     size=10000, alpha=0.05, fails=10, extra_args=None,
                     model_args=None):
        if model_args is None:
            model_args = {}
        model = build_model(dist, valuedomain, paramdomains, extra_args)
        domains = paramdomains.copy()
        for pt in product(domains, n_samples=100):
            pt = pm.Point(pt, model=model)
            pt.update(model_args)
            p = alpha
            # Allow KS test to fail (i.e., the samples be different)
            # a certain number of times. Crude, but necessary.
            f = fails
            while p <= alpha and f > 0:
                s0 = model.named_vars['value'].random(size=size, point=pt)
>               s1 = ref_rand(size=size, **pt)
E               TypeError: ref_rand() missing 1 required positional argument: 'lower'
pymc3/tests/test_distributions_random.py:38: TypeError

[gilliss]

@twiecki Thanks for checking in. I just made another push, 2cd14ab.

For this push, I merged the latest from pymc-devs:master, addressed the test-failing errors from the positional args in ref_rand, ran pytest on test_distributions_random.py, and tried out TruncatedNormal.dist(). Things seem to run as expected.

In summary this pr adds the members lower_check and upper_check to the TruncatedNormal class and also adds some tests for when just the upper or just the lower bound is set by the user.

[chang111]

@junpenglao Sorry, I don't consider so much. I appreciate what he has done. I will consider that in the future

[twiecki]

Alright, this looks good. See minor syntax fixes and it also needs a line in the release-notes (at the end of maintenance).

[gilliss]

@twiecki I've made the syntax adjustments. For release-notes, do I commit a new line in pymc3/docs/release-notes/pymc3-3.0.md?

In any case, the line could say "Set default lower and upper values of -inf and inf for pm.distributions.continuous.TruncatedNormal. This avoids errors caused by their previous values of None."

[twiecki]

Yes, that sounds great. You can also give add the issue number in that line..

[lucianopaz]

Wait, aren't we putting all of the release notes in RELEASE-NOTES.md at the root directory of the repo? What's that other release notes file in the docs?

[twiecki]

Oh that's right.

…

On Tue, Mar 26, 2019 at 5:46 PM Luciano Paz ***@***.***> wrote: Wait, aren't we putting all of the release notes in RELEASE-NOTES.md at the root directory of the repo? What's that other release notes file in the docs? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3250 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AApJmDEvJh50egUh7ogDk2k5WNOAPCNVks5vak7XgaJpZM4YcFqm> .

[gilliss]

OK, I'll undo my last and put the line in RELEASE-NOTES.md.

[twiecki]

Thanks @gilliss!