Skip to content

allow redefinition of inputs in logprob #192

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

Jump to bottom
Merged
merged 5 commits into from
Nov 22, 2020
Merged

allow redefinition of inputs in logprob #192

merged 5 commits into from
Nov 22, 2020

Conversation

@mohamed82008
Copy link
Contributor

@mohamed82008 mohamed82008 commented Nov 21, 2020

This PR fixes #190.

devmotion
devmotion requested changes Nov 21, 2020
View reviewed changes
Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

It would be good to remove the dependency on Turing in the tests. Additionally, the version should be bumped.

@devmotion
Copy link
Member

devmotion commented Nov 21, 2020

BTW it seems this fixes #2 as well?

@mohamed82008
Copy link
Contributor Author

mohamed82008 commented Nov 21, 2020

BTW it seems this fixes #2 as well?

Yes!

mohamed82008 and others added 2 commits November 21, 2020 22:45
@mohamed82008 @devmotion
Update test/prob_macro.jl
aec4d5b 
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
@mohamed82008
avoid Turing and revert pdf(..) change
4de6060
@mohamed82008
Copy link
Contributor Author

mohamed82008 commented Nov 21, 2020

I reverted the pdf change because the prob macro does exp(logpdf(..)) which gives a slightly different result from pdf in this case.

@devmotion
Copy link
Member

devmotion commented Nov 21, 2020

Just test for approximate equality? IMO that should be sufficient as well.

@mohamed82008
Copy link
Contributor Author

mohamed82008 commented Nov 21, 2020

Just test for approximate equality? IMO that should be sufficient as well.

It's fine as is.

devmotion
devmotion approved these changes Nov 22, 2020
View reviewed changes
Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

LGTM

@devmotion
Copy link
Member

devmotion commented Nov 22, 2020

bors r+

bors bot pushed a commit that referenced this pull request Nov 22, 2020
@mohamed82008
allow redefinition of inputs in logprob (#192)
b95dedd 
This PR fixes #190.
@bors
Copy link
Contributor

bors bot commented Nov 22, 2020

Timed out.

@devmotion
Copy link
Member

devmotion commented Nov 22, 2020

The time-out is caused by the missing Travis tests due to our negative credit balance. We have to decide if we want to ask for more credits or switch to Github Actions (or something else). But since all tests passed (apart from the Travis one), I'll merge this one manually.

@devmotion devmotion merged commit 9043d65 into master Nov 22, 2020
@devmotion devmotion deleted the mt/fix190 branch November 22, 2020 15:28
@devmotion devmotion mentioned this pull request Nov 22, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@devmotion devmotion devmotion approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Turing resamples variable that is passed as variable to condition on when querying

3 participants

@mohamed82008 @devmotion