Skip to content

Conversation

phipsgabler
Copy link
Member

The following simple evaluation does not currently work without Turing being loaded, since the tilde and assume/observe functions for the contexts provided by DynamicPPL are actually implemented in Turing:

using DynamicPPL@model coinflip(y) = begin
    p ~ Beta(1, 1)
    N = length(y)
    for i = 1:N
        y[i] ~ Bernoulli(p)
    end
end
model = coinflip([1,1,0])
vi = VarInfo()
model(vi)

As @mohamed82008 suggested, I basically just moved the "fallback methods" from Inference.jl over here. This required some additional movements:

  • I prefixed the tilde metafunctions in compiler.jl with "generate", to avoid name confusion
  • Some code had had to be split out into separate files and moved, to make imports work
  • The pseudo-distributions NoDist and NamedDist had to be moved to this package, since the assume/observe code depends on them (crucially, LikelihoodContext doesn't work without NoDist).

For testing, I didn't really now what to do and where to put it, except checking it does compile now. So I just created a new file for now.

I'd appreciate feedback on especially imports and other things I broke :) The Turing counterpart (removing the fallback methods) will follow soon.

@phipsgabler phipsgabler requested a review from mohamed82008 March 8, 2020 16:23
Copy link
Member

@trappmartin trappmartin left a comment

Choose a reason for hiding this comment

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

Look good to me. Thanks.

@phipsgabler
Copy link
Member Author

phipsgabler commented Mar 9, 2020

One thing I was worried about is removing NamedDist from Turing. Is that (supposed to be) used anywhere by externals?

@trappmartin
Copy link
Member

trappmartin commented Mar 9, 2020

Not that I'm aware of.

@mohamed82008
Copy link
Member

NamedDist is a feature. We should export it from Turing.

@mohamed82008
Copy link
Member

NamedDist is used by DiffEqBayes.

@phipsgabler
Copy link
Member Author

It will still be reexported from Turing. Same for assume/observe and their dot-variants.

@phipsgabler
Copy link
Member Author

The Turing side of the PR is now out as well.

I wonder how to test all this in a reasonable way. The Turing integration tests over here have certainly not caught a lot of the namespace errors I made initially -- those I only discovered by testing Turing itself with this branch of DynamicPPL.

@devmotion
Copy link
Member

There's some discussion about the test setup in #18. I guess ideally DynamicPPL should be tested without depending on Turing.

@phipsgabler
Copy link
Member Author

I just saw that this relates to #19 as well.

@phipsgabler phipsgabler changed the title Make default model evaluation independent of Turing [WIP?] Make default model evaluation independent of Turing Mar 15, 2020
@trappmartin
Copy link
Member

@mohamed82008 can this be merged?

@devmotion
Copy link
Member

I guess it would be nice if the tests pass. The Libtask error should be fixed by now, I guess, but I assume one has to update the Turing folder in the test directory with the changes in TuringLang/Turing.jl#1151 (which should maybe be updated with master since there might have been some changes to Turing master in the mean time).

@mohamed82008
Copy link
Member

I will review this tonight.

@phipsgabler
Copy link
Member Author

@devmotion Travis is still failing solely due to the Libtask error. On my system, all DynamicPPL tests pass with Libtask fixed to the older version. From the other side, also Turing's test pass for TuringLang/Turing.jl#1151 being used together with this branch.

I would have liked to update the tests already, of course, but right now I consider that all a bit too fragile for me to touch it.

@devmotion
Copy link
Member

devmotion commented Mar 16, 2020

Hmm it seems that the tests still used Libtask 0.3.2, I thought the problem should be fixed with Libtask 0.3.3?

@phipsgabler
Copy link
Member Author

phipsgabler commented Mar 16, 2020

Right -- appearently those test results are still from 5 days ago. Is there a way to trigger Travis manually? Found it, I wasn't signed in.

@phipsgabler
Copy link
Member Author

Now it's just Travis killing the job because it takes so long...

@devmotion
Copy link
Member

No, the problem is that the logs are too long. The problem is that the Turing test folder in DynamicPPL has not been updated with the latest changes in Turing, in particular TuringLang/Turing.jl#1156.

@mohamed82008
Copy link
Member

Why not update it?

Copy link
Member

@mohamed82008 mohamed82008 left a comment

Choose a reason for hiding this comment

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

LGTM. But let's make sure the tests pass.

@phipsgabler
Copy link
Member Author

phipsgabler commented Mar 17, 2020

Alright, the Turing test folder is now updated to the state of the https://github.com/phipsgabler/Turing.jl/tree/context_independence, which is up to date with the current Turing master. All tests pass on my machine, let's see what Travis says :)

@codecov
Copy link

codecov bot commented Mar 17, 2020

Codecov Report

Merging #43 into master will decrease coverage by 7.33%.
The diff coverage is 59.72%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #43      +/-   ##
=========================================
- Coverage   88.24%   80.9%   -7.34%     
=========================================
  Files           9      12       +3     
  Lines         723     880     +157     
=========================================
+ Hits          638     712      +74     
- Misses         85     168      +83
Impacted Files Coverage Δ
src/DynamicPPL.jl 100% <ø> (ø) ⬆️
src/compiler.jl 94.28% <ø> (+0.39%) ⬆️
src/varinfo.jl 87.8% <100%> (-0.04%) ⬇️
src/utils.jl 32.35% <25%> (-4.02%) ⬇️
src/distribution_wrappers.jl 25% <25%> (ø)
src/context_implementations.jl 50.75% <50.75%> (ø)
src/varname.jl 92.85% <92.85%> (ø)
... and 1 more

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 c59f519...ee9b693. Read the comment docs.

@mohamed82008 mohamed82008 merged commit fe368b9 into TuringLang:master Mar 18, 2020
@mohamed82008
Copy link
Member

Thanks @phipsgabler , great work!

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