Skip to content
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

Add more tests for single distributions #621

Merged
merged 15 commits into from
Jan 27, 2019
Merged

Conversation

xukai92
Copy link
Member

@xukai92 xukai92 commented Dec 10, 2018

  • Uni
  • Multi
  • Matrix

test/models.jl/uni.jl Outdated Show resolved Hide resolved
test/models.jl/uni.jl Outdated Show resolved Hide resolved
@willtebbutt
Copy link
Member

I appreciate that this is a WIP, but I've added a few comments anyway. Is there an obvious way to construct some logpdf tests? I'm not really sure what the easiest way to go about this is, but it would be cool if we could do something.

@xukai92
Copy link
Member Author

xukai92 commented Dec 24, 2018

By logpdf tests do you refer to more complex models or single distributions?

@willtebbutt
Copy link
Member

By logpdf tests do you refer to more complex models or single distributions?

Fair point. If we're only considering simple distributions in this PR then we are essentially just trying to ensure that Turing hasn't screwed anything up and, therefore, can just compare against the Distributions.jl logpdf for the component distributions.

@xukai92
Copy link
Member Author

xukai92 commented Dec 24, 2018

Yes. Actually I'd prefer to add them into this https://github.com/TuringLang/Turing.jl/blob/master/test/ad.jl/AD_compatibility_with_distributions.jl file, as they didn't need any sampling. Also, maybe we can leverage https://github.com/JuliaStats/ConjugatePriors.jl to construct tests more than single distributions - I will try to do this as well.

@xukai92
Copy link
Member Author

xukai92 commented Jan 3, 2019

@mohamed82008 I kept seeing numerical errors from Bijectors.jl (https://travis-ci.org/TuringLang/Turing.jl/jobs/474636293#L873) when sampling for single Dirichlet distribution with NUTS. I didn't manage to figure out what happens here - any idea?

@xukai92
Copy link
Member Author

xukai92 commented Jan 3, 2019

I guess it may be related to this test case https://github.com/TuringLang/Bijectors.jl/blob/master/test/transform.jl#L51, for which in Turing.jl we don't have a special rand for those dimensions belonging to Dirichlet distributions.

Sorry this is irrelevant.

@xukai92
Copy link
Member Author

xukai92 commented Jan 3, 2019

I found a possible cause and created an issue in Bijectors.jl (see TuringLang/Bijectors.jl#12).

@xukai92
Copy link
Member Author

xukai92 commented Jan 9, 2019

@yebai @willtebbutt As the numerical issue on Dirichlet is a separate issue, can we merge this PR if there is no objection?

@yebai
Copy link
Member

yebai commented Jan 9, 2019

@mohamed82008 Could you take a look at the numerical issues identified by this PR? Ideally, we should fix them before merging this PR.

@mohamed82008
Copy link
Member

Sure thing. I was planning to take a look a while back but forgot! I will see if I can reproduce the error first.

@mohamed82008
Copy link
Member

All tests pass on my machine.

@yebai
Copy link
Member

yebai commented Jan 10, 2019

All tests pass on my machine.

I've seenBeta and Gamma tests randomly break on Travis, see

https://travis-ci.org/TuringLang/Turing.jl/jobs/475012349

@xukai92
Copy link
Member Author

xukai92 commented Jan 10, 2019

I guess it's because of the end-of-window update that tunes the step size to a very high value (https://travis-ci.org/TuringLang/Turing.jl/jobs/475012349#L780). Let me cap that as well.

@yebai
Copy link
Member

yebai commented Jan 14, 2019

@xukai92 all the tests are now passing. Do you want to resolve Mohamed's comment before this PR gets merged?

@xukai92
Copy link
Member Author

xukai92 commented Jan 14, 2019

@yebai Thanks for the reminding - resolved.

@willtebbutt
Copy link
Member

@xukai92 I don't want to too much additional work as this PR looks great, but there do appear to be a few distributions missing from this list e.g. InverseWishart. Is there a particular reason for this?

@xukai92
Copy link
Member Author

xukai92 commented Jan 15, 2019

@willtebbutt I added more dists now!

@xukai92
Copy link
Member Author

xukai92 commented Jan 15, 2019

Samples from VonMises(0, 1) seems to be biased (2 runs of tests on my local fails).

src/samplers/adapt/stepsize.jl Outdated Show resolved Hide resolved
test/models.jl/single_dist_correctness.jl Outdated Show resolved Hide resolved
@mohamed82008 mohamed82008 force-pushed the kx/distributions-tests branch from 8cb6823 to 42d4d4a Compare January 26, 2019 12:31
@mohamed82008
Copy link
Member

Rebased on master.

@xukai92
Copy link
Member Author

xukai92 commented Jan 26, 2019

Let's merge this one if not objections

@mohamed82008
Copy link
Member

@yebai It would be nice to merge this before the other small PRs which are also ready to merge.

@xukai92 xukai92 merged commit c8e40c5 into master Jan 27, 2019
@xukai92
Copy link
Member Author

xukai92 commented Jan 27, 2019

@mohamed82008 I merged it :)

@mohamed82008 mohamed82008 deleted the kx/distributions-tests branch February 9, 2019 09:40
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.

4 participants