-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conditional autoregression distribution #4504
Conversation
This looks like a great start! |
Regarding tests, yeah, pointwise is probably good enough here if we don't have a reference implementation. |
The test that I wrote for this distribution doesn't fit neatly into the framework already set up for testing against Scipy because while the CAR logp is defined to be that of a multivariate normal with a specific covariance structure, the implementation here isn't exactly equal - it differs from the MVN logp by an additive constant. |
I think all of the suggested changes have been addressed. Is the preferred workflow to individually mark each conversation as 'resolved' once the changes have been made, or should that be done by the reviewer? |
Best to let @ricardoV94 resolve them. |
Added reference for CAR distribution Running precommit checks Adding CAR test and updating CAR logp Extending CAR test to try multiple shapes Docstring change for CAR logp Adding bounds to logp for CAR distribution
…to car-distribution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's looking great.
I left another comment. I'll resolve the old ones now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
) | ||
|
||
def random(self, point=None, size=None): | ||
raise NotImplementedError("Sampling from a CAR distribution is not supported.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we turn this into a feature request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I've got a few leads on efficient methods for doing this. We can track that discussion at #4518.
Thanks for the reviews, @ricardoV94 and @twiecki! |
* Add conditional autoregression distribution (CAR)
* Add conditional autoregression distribution (CAR)
After many months of dragging my feet, I finally spent some time cleaning up the CAR implementation that @junpenglao showed in an example notebook (Issue #3689). I have modified that code to allow for multiple observations of a CAR-distributed vector conditional on a single fixed value for the adjacency matrix W.
This is definitely a work in progress and I am humbly asking for some assistance in devising a testing strategy for this distribution. Right now, all I can think of is to do pointwise tests on the
logp
for fixed values of the inputs.