Skip to content

Restrict domain on alpha in the CAR distribution #6801

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.

Sign up for GitHub

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 4 commits into from
Jul 26, 2023
Merged

Restrict domain on alpha in the CAR distribution #6801

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1f00345
Restricted alpha to be greater than -1 and less than 1. Adjusted para…
daniel-saunders-phil Jun 28, 2023
835f533
Ricardo's suggestions
daniel-saunders-phil Jun 28, 2023
eba76f6
add tests for code-cov
daniel-saunders-phil Jul 6, 2023
26859b1
logp test, test rename, np.all - > np.any
daniel-saunders-phil Jul 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions pymc/distributions/multivariate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2061,7 +2061,10 @@ def make_node(self, rng, size, dtype, mu, W, alpha, tau):
W = Assert(msg)(W, pt.allclose(W, W.T))

tau = pt.as_tensor_variable(floatX(tau))

alpha = pt.as_tensor_variable(floatX(alpha))
msg = "the domain of alpha is: -1 < alpha < 1."
alpha = Assert(msg)(alpha, pt.lt(alpha, 1) and pt.gt(alpha, -1))
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't put this assert here. Instead raise an error in the rng_fn if the alpha is actually problematic for random draws.

Otherwise Asserts will complicate the graph needlessly.

Anyway the reason you don't see the exception is probably that you need to use pt.and_ instead of python and.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup the pt.and_ was what I was missing. So instead of using assert in the rng function, I just raise a ValueError? Is that right?

if alpha >= 1 or alpha <= -1:
            raise ValueError("the domain of alpha is: -1 < alpha < 1")

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but alpha could be a vector so you need a np.all(...)

Copy link
Contributor Author

@daniel-saunders-phil daniel-saunders-phil Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

gotcha - okay sent in a new commit with these changes.


return super().make_node(rng, size, dtype, mu, W, alpha, tau)

Expand Down Expand Up @@ -2143,8 +2146,9 @@ class CAR(Continuous):
:func:`~pytensor.sparse.basic.as_sparse_or_tensor_variable` is
used for this sparse or tensorvariable conversion.
alpha : tensor_like of float
Autoregression parameter taking values between -1 and 1. Values closer to 0 indicate weaker
correlation and values closer to 1 indicate higher autocorrelation. For most use cases, the
Autoregression parameter taking values greater than -1 and less than 1.
Values closer to 0 indicate weaker correlation and values closer to
1 indicate higher autocorrelation. For most use cases, the
support of alpha should be restricted to (0, 1).
tau : tensor_like of float
Positive precision variable controlling the scale of the underlying normal variates.
Expand Down Expand Up @@ -2211,10 +2215,10 @@ def logp(value, mu, W, alpha, tau):
logquad = (tau * delta * tau_dot_delta).sum(axis=-1)
return check_parameters(
0.5 * (logtau + logdet - logquad),
-1 <= alpha,
alpha <= 1,
-1 < alpha,
alpha < 1,
tau > 0,
msg="-1 <= alpha <= 1, tau > 0",
msg="-1 < alpha < 1, tau > 0",
)


Expand Down