Skip to content

Convert continuous and discrete distribution parameters to floatX or int32 #3300

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Jan 3, 2019

Conversation

lucianopaz
Copy link
Member

This should solve issue #3223. While I was working on PR #3293 I also ran into this problem but decided to submit a separate PR for it. I only explicitly casted to floatX or int32 if the distribution's docstring said that the parameter should be a float or an int. The rest (multivariate, mixture and timeseries) I left unchanged.

@twiecki
Copy link
Member

twiecki commented Dec 22, 2018

Needs a rebase.

@lucianopaz
Copy link
Member Author

@twiecki, some tests are failing when FLOATX='float32'. Should we mark those as expected failures?

@lucianopaz
Copy link
Member Author

By chance I saw #2366, where it says that int32 should be used if floatX is float64, and that int16 should be used if float32 was set as floatX. Maybe we should do the same here.

…xed test_distributions.py errors. Attempted a fix to test_variational_inference.py errors, which I cannot reproduce locally.
self.sigma = self.sd = sigma = tt.as_tensor_variable(sigma)
self.nu = nu = tt.as_tensor_variable(nu)
self.mu = mu = tt.as_tensor_variable(floatX(mu))
self.sigma = sigma = tt.as_tensor_variable(floatX(sigma))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.sigma = sigma = tt.as_tensor_variable(floatX(sigma))
self.sigma = self.sd = sigma = tt.as_tensor_variable(floatX(sigma))

r'$\text{alpha} \sim \text{Normal}(\mathit{mu}=0,~\mathit{sigma}=10.0)$',
r'$\text{sigma} \sim \text{HalfNormal}(\mathit{sigma}=1.0)$',
r'$\text{alpha} \sim \text{Normal}(\mathit{mu}=0.0,~\mathit{sd}=10.0)$',
r'$\text{sigma} \sim \text{HalfNormal}(\mathit{sd}=1.0)$',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
r'$\text{sigma} \sim \text{HalfNormal}(\mathit{sd}=1.0)$',
r'$\text{sigma} \sim \text{HalfNormal}(\mathit{sigma}=1.0)$',

@@ -1298,11 +1295,11 @@ def setup_class(self):
Y_obs = Normal('Y_obs', mu=mu, sigma=sigma, observed=Y)
self.distributions = [alpha, sigma, mu, b, Y_obs]
self.expected = (
r'$\text{alpha} \sim \text{Normal}(\mathit{mu}=0,~\mathit{sigma}=10.0)$',
r'$\text{sigma} \sim \text{HalfNormal}(\mathit{sigma}=1.0)$',
r'$\text{alpha} \sim \text{Normal}(\mathit{mu}=0.0,~\mathit{sd}=10.0)$',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
r'$\text{alpha} \sim \text{Normal}(\mathit{mu}=0.0,~\mathit{sd}=10.0)$',
r'$\text{alpha} \sim \text{Normal}(\mathit{mu}=0.0,~\mathit{sigma}=10.0)$',

r'$\text{mu} \sim \text{Deterministic}(\text{alpha},~\text{Constant},~\text{beta})$',
r'$\text{beta} \sim \text{Normal}(\mathit{mu}=0,~\mathit{sigma}=10.0)$',
r'$\text{Y_obs} \sim \text{Normal}(\mathit{mu}=\text{mu},~\mathit{sigma}=f(\text{sigma}))$'
r'$\text{beta} \sim \text{Normal}(\mathit{mu}=0.0,~\mathit{sd}=10.0)$',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
r'$\text{beta} \sim \text{Normal}(\mathit{mu}=0.0,~\mathit{sd}=10.0)$',
r'$\text{beta} \sim \text{Normal}(\mathit{mu}=0.0,~\mathit{sigma}=10.0)$',

r'$\text{beta} \sim \text{Normal}(\mathit{mu}=0,~\mathit{sigma}=10.0)$',
r'$\text{Y_obs} \sim \text{Normal}(\mathit{mu}=\text{mu},~\mathit{sigma}=f(\text{sigma}))$'
r'$\text{beta} \sim \text{Normal}(\mathit{mu}=0.0,~\mathit{sd}=10.0)$',
r'$\text{Y_obs} \sim \text{Normal}(\mathit{mu}=f(\text{mu}),~\mathit{sd}=f(\text{sigma}))$'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
r'$\text{Y_obs} \sim \text{Normal}(\mathit{mu}=f(\text{mu}),~\mathit{sd}=f(\text{sigma}))$'
r'$\text{Y_obs} \sim \text{Normal}(\mathit{mu}=f(\text{mu}),~\mathit{sigma}=f(\text{sigma}))$'

@twiecki
Copy link
Member

twiecki commented Jan 3, 2019


    def test__repr_latex_(self):
        for distribution, tex in zip(self.distributions, self.expected):
>           assert distribution._repr_latex_() == tex
E           AssertionError: assert '$\\text{Y_ob...ext{sigma}))$' == '$\\text{Y_obs...ext{sigma}))$'
E             - $\text{Y_obs} \sim \text{Normal}(\mathit{mu}=\text{mu},~\mathit{sigma}=f(\text{sigma}))$
E             + $\text{Y_obs} \sim \text{Normal}(\mathit{mu}=f(\text{mu}),~\mathit{sigma}=f(\text{sigma}))$
E             ?                                              ++         +

@twiecki twiecki merged commit ffcd50f into pymc-devs:master Jan 3, 2019
@twiecki
Copy link
Member

twiecki commented Jan 3, 2019

Thanks @lucianopaz!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants