Skip to content

Conversation

@mohamed82008
Copy link
Contributor

This PR fixes the SampleFromUniform code. SampleFromUniform is only useful in initializing the HMC state. This means that:

  1. A new value should always be sampled overwriting the existing values in VarInfo which may have come from SampleFromPrior, and
  2. The "trans" flag should be set to true because 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.

@mohamed82008 mohamed82008 force-pushed the mt/fix_samplefromuniform branch from 0a122df to cabc01e Compare April 20, 2020 18:09
Co-Authored-By: David Widmann <devmotion@users.noreply.github.com>
Copy link
Member

@yebai yebai left a 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.

@mohamed82008
Copy link
Contributor Author

Yes the broken tests test the old behavior. I think removing them should be ok.

@devmotion
Copy link
Member

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).

@mohamed82008
Copy link
Contributor Author

Sure, I will finish it tonight.

@devmotion
Copy link
Member

Thanks!

@codecov
Copy link

codecov bot commented Apr 27, 2020

Codecov Report

Merging #78 into master will decrease coverage by 0.54%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
src/utils.jl 53.06% <50.00%> (ø)
src/context_implementations.jl 56.46% <54.54%> (+1.99%) ⬆️
src/varinfo.jl 83.90% <100.00%> (-0.53%) ⬇️
src/compiler.jl 87.82% <0.00%> (-0.21%) ⬇️
src/varname.jl 76.47% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7159cb...a31e523. Read the comment docs.

@mohamed82008
Copy link
Contributor Author

Once tests pass, this can be merged. I will sleep now.

@devmotion
Copy link
Member

I'm sure tests will fail on Julia master, at least that's what happened in all other PRs in the last days.

@yebai yebai merged commit 26d90d7 into master Apr 27, 2020
@yebai yebai deleted the mt/fix_samplefromuniform branch April 27, 2020 19:12
@yebai yebai restored the mt/fix_samplefromuniform branch April 27, 2020 19:12
@yebai yebai deleted the mt/fix_samplefromuniform branch May 25, 2020 11:25
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.

5 participants