Skip to content

Conversation

@torfjelde
Copy link
Member

@torfjelde torfjelde commented Jun 10, 2021

This PR makes Turing compatible with [email protected].

EDIT: This should go after #1633 .

torfjelde and others added 30 commits June 8, 2021 06:22
Co-authored-by: David Widmann <[email protected]>
@yebai
Copy link
Member

yebai commented Jun 10, 2021

I'm a bit surprised that we need to overload tilde_* functions for ESS and MAP. It seems these algorithms only require a subset of three operations on the model, i.e. logprior, loglikelihood and logdensity/logjoint. Since these operations are already defined in DynamicPPL using contexts, is there a way to remove such overloads, or only overload sampler-facing functions like assume/observe?

@torfjelde
Copy link
Member Author

torfjelde commented Jun 10, 2021

Likely yeah, but IMO this should be done in a separate PR? This PR should be "minimal change necessary to make Turing.jl compatible with new DPPL".

@torfjelde
Copy link
Member Author

torfjelde commented Jun 10, 2021

Also, it seems like SMC samplers are now broken? o.O Do any of you @yebai @devmotion have any idea as to why this is? I'm not at all familiar with APS so it's a bit difficult to debug.

EDIT: Just discovered there's a TracedModel. Think I'm on to something.

EDIT 2: Figured it out. observe(::Samler, ...) is never hit because DynamicPPL.tilde_observe(::DefaultContext, sampler, ...) = observe(...), i.e. requires a change in DPPL.

@yebai
Copy link
Member

yebai commented Jun 10, 2021

Likely yeah, but IMO this should be done in a separate PR? This PR should be "minimal change necessary to make Turing.jl compatible with new DPPL".

Sounds good.

EDIT 2: Figured it out. observe(::Samler, ...) is never hit because DynamicPPL.tilde_observe(::DefaultContext, sampler, ...) = observe(...), i.e. requires a change in DPPL.

Great - I'll take a look at the DPPL PR.

bors bot pushed a commit to TuringLang/DynamicPPL.jl that referenced this pull request Jun 10, 2021
In the recent breaking release we mistakenly removed the call to `observe(::AbstractSampler, right left, vi)` as a fallback for `DefaultContext`, leading to certain sampler breaking in Turing (TuringLang/Turing.jl#1636). 

This PR adds back a proper fallback, making overloads such as https://github.com/TuringLang/Turing.jl/blob/tor%2Fdppl-update/src/inference/AdvancedSMC.jl#L353-L356 work as before 0.11.
@codecov
Copy link

codecov bot commented Jun 10, 2021

Codecov Report

Merging #1636 (d90ed39) into master (9f52d75) will increase coverage by 0.29%.
The diff coverage is 66.66%.

❗ Current head d90ed39 differs from pull request most recent head 46a00a8. Consider uploading reports for the commit 46a00a8 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1636      +/-   ##
==========================================
+ Coverage   79.32%   79.61%   +0.29%     
==========================================
  Files          23       23              
  Lines        1422     1413       -9     
==========================================
- Hits         1128     1125       -3     
+ Misses        294      288       -6     
Impacted Files Coverage Δ
src/modes/ModeEstimation.jl 84.29% <57.14%> (+3.52%) ⬆️
src/inference/ess.jl 98.00% <85.71%> (ø)

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 9f52d75...46a00a8. Read the comment docs.

@coveralls
Copy link

coveralls commented Jun 10, 2021

Pull Request Test Coverage Report for Build 925470820

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 14 of 21 (66.67%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 79.618%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/inference/ess.jl 6 7 85.71%
src/modes/ModeEstimation.jl 8 14 57.14%
Totals Coverage Status
Change from base Build 925007883: 0.3%
Covered Lines: 1125
Relevant Lines: 1413

💛 - Coveralls

@torfjelde
Copy link
Member Author

Ready with the 👍 once the last test passes?

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.

Looks good, just bump the Turing version number? Do we want it to be a breaking release as well?

@torfjelde
Copy link
Member Author

Good quesh. Technically nothing that we export is breaking, buuuut we're exporting DynamicPPL so I don't know how that works.

@torfjelde
Copy link
Member Author

torfjelde commented Jun 10, 2021

Maybe make it breaking just on the off-chance someone is actually overloading Turing.tilde, etc.?

EDIT: I misremebered, Turing.tilde isn't available, but Turing.assume is. Buuuut Turing.assume isn't changed, or rather than changes we've made doesn't break the current assume. Soooo non-breaking?

@devmotion
Copy link
Member

As long as we do not export the methods that were broken in DynamicPPL, technically it is non-breaking. E.g., if we would reexport DynamicPPL or any of these methods it would be clear. It is a bit unclear how to deal with an exported module such as DynamicPPL... But generally if methods are only available internally as Turing.tilde etc. it does not require a breaking release.

@torfjelde
Copy link
Member Author

The boundary is blurred further by the fact that most people don't about DPPL but are using stuff from there directly through Turing.whatever.

BUT I was mistaken; we don't have Turing.tilde, etc. available, so we should be good 👍

@torfjelde torfjelde merged commit 57e0512 into master Jun 10, 2021
@delete-merged-branch delete-merged-branch bot deleted the tor/dppl-update branch June 10, 2021 14: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.

5 participants