[WIP] Fix support for distributions with stochastic support #115

mohamed82008 · 2020-05-13T17:19:47Z

This PR fixes support for distributions with stochastic support in models. I just want to add some more tests.

codecov · 2020-05-13T17:34:18Z

Codecov Report

Merging #115 into master will decrease coverage by 4.18%.
The diff coverage is 60.14%.

@@            Coverage Diff             @@
##           master     #115      +/-   ##
==========================================
- Coverage   74.83%   70.65%   -4.19%     
==========================================
  Files          17       18       +1     
  Lines         906     1046     +140     
==========================================
+ Hits          678      739      +61     
- Misses        228      307      +79

Impacted Files	Coverage Δ
src/DynamicPPL.jl	`100.00% <ø> (ø)`
src/compat/ad.jl	`0.00% <0.00%> (ø)`
src/prob_macro.jl	`92.30% <ø> (-0.08%)`	⬇️
src/threadsafe.jl	`22.64% <0.00%> (-5.93%)`	⬇️
src/context_implementations.jl	`54.28% <47.61%> (-1.35%)`	⬇️
src/varinfo/utils.jl	`72.51% <55.71%> (-10.44%)`	⬇️
src/varinfo/linking.jl	`69.73% <62.90%> (-18.73%)`	⬇️
src/varinfo/indexing.jl	`77.51% <68.67%> (-14.59%)`	⬇️
src/varinfo/ad.jl	`100.00% <100.00%> (ø)`
src/varinfo/types.jl	`73.68% <100.00%> (+3.09%)`	⬆️
... and 3 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 7ab6615...4d881ea. Read the comment docs.

devmotion

I'm sorry but I don't think I'm able to review this PR properly, due to the new file structure it is very difficult to figure out which parts of the VarInfo implementation are actually modified and which are just moved to some other file. Can you please keep the VarInfo implementation in varinfo.jl for now and split it in a separate PR (I'm very much in favour of this refactoring but it should be done separately)?

Some general comments so far:

It seems this PR adds a bunch of new functions to the VarInfo and ThreadedVarInfo interface. As discussed before, I think we should rather try to simplify the implementation and reduce the number of functions, so we should evaluate carefully if all of them are needed.
I don't think the definition of value for the AD backends belongs to DynamicPPL (and hence Requires is not needed, see my comment below).
I think we should perform some detailed benchmarking, just to ensure that there are no performance regressions. I'm not sure if we have some standard benchmarking suite set up for Turing? Isn't there a bot for it?

devmotion · 2020-05-13T18:29:08Z

src/varinfo/ad.jl

+function __init__()
+    @require ForwardDiff = "f6369f11-7733-5829-9624-2563aa707210" begin
+        value(x::ForwardDiff.Dual) = ForwardDiff.value(x)
+        value(x::AbstractArray{<:ForwardDiff.Dual}) = ForwardDiff.value.(x)
+    end
+    @require ReverseDiff = "37e2e3b7-166d-5795-8a7a-e32c996b4267" begin
+        value(x::ReverseDiff.TrackedReal) = ReverseDiff.value(x)
+        value(x::ReverseDiff.TrackedArray) = ReverseDiff.value(x)
+        value(x::AbstractArray{<:ReverseDiff.TrackedReal}) = ReverseDiff.value.(x)
+    end
+    @require Tracker = "9f7883ad-71c0-57eb-9f7f-b5c9e6d3789c" begin
+        value(x::Tracker.TrackedReal) = Tracker.data(x)
+        value(x::Tracker.TrackedArray) = Tracker.data(x)
+        value(x::AbstractArray{<:Tracker.TrackedReal}) = Tracker.data.(x)
+    end
+end


These definitions should not be part of DynamicPPL but moved to some other package. The only AD-related part in DynamicPPL should be custom adjoints for functions defined in DynamicPPL if required, which should be implemented without depending on the AD packages if possible (by using, e.g., ZygoteRules or ChainRulesCore). If these definitions are removed, there's also no need for adding Requires as a dependency, AFAICT.

mohamed82008 · 2020-05-13T19:05:20Z

I'm sorry but I don't think I'm able to review this PR properly, due to the new file structure it is very difficult to figure out which parts of the VarInfo implementation are actually modified and which are just moved to some other file. Can you please keep the VarInfo implementation in varinfo.jl for now and split it in a separate PR (I'm very much in favour of this refactoring but it should be done separately)?

Thanks for your comments @devmotion. I will split the files first in a PR.

These definitions should not be part of DynamicPPL but moved to some other package. The only AD-related part in DynamicPPL should be custom adjoints for functions defined in DynamicPPL if required, which should be implemented without depending on the AD packages if possible (by using, e.g., ZygoteRules or ChainRulesCore). If these definitions are removed, there's also no need for adding Requires as a dependency, AFAICT.

The DynamicPPL.value function is needed by DynamicPPL. Putting it in a separate package means we would have to depend on that other package which depends on Requires so it means more rather than less dependencies.

I think we should perform some detailed benchmarking, just to ensure that there are no performance regressions. I'm not sure if we have some standard benchmarking suite set up for Turing? Isn't there a bot for it?

I did some benchmarking on my machine and there was no regression as far as I can tell. I will do more and report here. There is a benchmarking bot last modified by @KDr2 I believe. Unfortunately, I don't think it's exhaustive so I will use the benchmarks from TuringExamples for now.

As for the added complexity, I am not sure what I can do about that. I can rethink the need for some functions by trying to update the internals' docs. While doing that, I might either justify each and every function or find some to be replaceable. Either way, we would be in a better situation than we are now :)

mohamed82008 · 2020-05-13T20:47:40Z

@yebai did you close this on purpose?

yebai · 2020-05-13T20:51:19Z

@yebai did you close this on purpose?

yes, sorry, I have do temporarily close this PR to revert some changes on the master.

mohamed82008 · 2020-05-13T20:52:38Z

yes, sorry, I have do temporarily close this PR to revert some changes on the master.

Hmm I don't see how they are related. What changes do you want to revert? The files reorganization?

yebai · 2020-05-13T20:53:46Z

Hmm I don't see how they are related. What changes do you want to revert? The files reorganization?

yes, Github does not seem to allow reverting changes on master with open PRs.

mohamed82008 · 2020-05-13T20:55:00Z

yes, Github does not seem to allow reverting changes on master with open PRs.

You can reset the last commit and force push to master if you really want. May I ask why you want to revert them? The varinfo file is getting unwieldy. I am really in favor of that reorganization.

yebai · 2020-05-13T20:59:32Z

You can reset the last commit and force push to master if you really want. May I ask why you want to revert them? The varinfo file is getting unwieldy. I am really in favor of that reorganization.

I commented on this in another PR. I got your point for reorganisation. I think it's better to stick with the single-file organisation for now. I discussed with Kai in AdvancedHMC about this too. I feel it is easier to track changes with all functionalities related to one data structure in one file; it is ok to separate code by sections if needed. I am happy to revisit this organisation issue in the future if necessary, but better to have some team-wide discussions first.

yebai · 2021-01-28T17:02:22Z

I'm closing this PR for now since we don't intend to support distributions with stochastic support for now - it might change in the future if there is a strong case. We'll revisit when that happens.

mohamed82008 requested a review from devmotion May 13, 2020 17:45

devmotion requested changes May 13, 2020

View reviewed changes

mohamed82008 mentioned this pull request May 13, 2020

Reorganize varinfo files #116

Merged

mohamed82008 added 2 commits May 14, 2020 06:25

squash commit

6782e36

test fixes

4d881ea

mohamed82008 force-pushed the mt/fix_stochastic_support branch from 7fd78f1 to 4d881ea Compare May 13, 2020 20:43

yebai closed this May 13, 2020

yebai reopened this May 13, 2020

This was referenced May 14, 2020

Gripes and anti-gripes about VarInfo #119

Closed

Add MLE/MAP functionality TuringLang/Turing.jl#1230

Merged

mgmverburg mentioned this pull request Nov 26, 2020

Querying from a chain given an explicitly ignored variable throws an error #198

Closed

yebai closed this Jan 28, 2021

yebai mentioned this pull request Dec 16, 2021

Formalise VarInfo/AbstractVarInfo API #68

Closed

yebai deleted the mt/fix_stochastic_support branch January 28, 2022 20:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[WIP] Fix support for distributions with stochastic support #115

[WIP] Fix support for distributions with stochastic support #115

Uh oh!

mohamed82008 commented May 13, 2020

Uh oh!

codecov bot commented May 13, 2020 •

edited

Loading

Uh oh!

devmotion left a comment

Uh oh!

devmotion May 13, 2020

Uh oh!

mohamed82008 commented May 13, 2020

Uh oh!

mohamed82008 commented May 13, 2020

Uh oh!

yebai commented May 13, 2020

Uh oh!

mohamed82008 commented May 13, 2020

Uh oh!

yebai commented May 13, 2020

Uh oh!

mohamed82008 commented May 13, 2020

Uh oh!

yebai commented May 13, 2020

Uh oh!

yebai commented Jan 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[WIP] Fix support for distributions with stochastic support #115

[WIP] Fix support for distributions with stochastic support #115

Uh oh!

Conversation

mohamed82008 commented May 13, 2020

Uh oh!

codecov bot commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

devmotion left a comment

Choose a reason for hiding this comment

Uh oh!

devmotion May 13, 2020

Choose a reason for hiding this comment

Uh oh!

mohamed82008 commented May 13, 2020

Uh oh!

mohamed82008 commented May 13, 2020

Uh oh!

yebai commented May 13, 2020

Uh oh!

mohamed82008 commented May 13, 2020

Uh oh!

yebai commented May 13, 2020

Uh oh!

mohamed82008 commented May 13, 2020

Uh oh!

yebai commented May 13, 2020

Uh oh!

yebai commented Jan 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented May 13, 2020 •

edited

Loading