-
Notifications
You must be signed in to change notification settings - Fork 37
Fix SampleFromUniform #78
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
Conversation
0a122df to
cabc01e
Compare
Co-Authored-By: David Widmann <devmotion@users.noreply.github.com>
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.
@mohamed82008 Looks good to me, but seems some tests are broken due to these changes.
|
Yes the broken tests test the old behavior. I think removing them should be ok. |
|
Can we finish this PR? I'm waiting for these fixes to adjust/correct the tests in #77 (if that PR is supposed to be merged). |
|
Sure, I will finish it tonight. |
|
Thanks! |
…ang/DynamicPPL.jl into mt/fix_samplefromuniform
Codecov Report
@@ Coverage Diff @@
## master #78 +/- ##
==========================================
- Coverage 77.46% 76.92% -0.55%
==========================================
Files 13 13
Lines 821 845 +24
==========================================
+ Hits 636 650 +14
- Misses 185 195 +10
Continue to review full report at Codecov.
|
|
Once tests pass, this can be merged. I will sleep now. |
|
I'm sure tests will fail on Julia master, at least that's what happened in all other PRs in the last days. |
This PR fixes the SampleFromUniform code. SampleFromUniform is only useful in initializing the HMC state. This means that:
VarInfowhich may have come fromSampleFromPrior, and"trans"flag should be set totruebecause we are not sampling in the support of the random variables' distributions.This PR also fixes the uniform distribution's support as per @xukai92's suggestion.