You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I think some exposition is required here, and I didn't have time to explain this super clearly during the meeting.
The API of DITest is like this:
You construct a scenario, which includes the function f, the value at which to evaluate it / the gradient x, and a bunch of other things. Crucially, the scenario does not include the adtype.
You then run the scenario with an adtype (or an array thereof).
From the perspective of generic functions f, this is quite a nice interface. The tricky bit with DynamicPPL, as I briefly mentioned, is that when you pass LogDensityFunction a model, varinfo, etc. it does a bunch of things that not only changes the function f being differentiated, but also potentially modifies the adtype that is actually used. See, especially, this constructor:
(Note that LogDensityFunctionsAD.jl used to do this stuff for us; #806 effectively removed it and inlined its optimisations into that inner constructor.)
What this means is that, to be completely consistent with the way DynamicPPL behaves, one has to:
Reproduce the code inside src/logdensityfunctions.jl that generates the function f, so that the scenario can use the correct f.
Because the above depends on the adtype, we have to make sure that scenarios generated with one adtype are later run with the same adtype.
In fact, the preparation in the LogDensityFunction doesn't only depend on the adtype; it potentially also modifies the adtype.
That's why this PR doesn't just include make_scenario; it also includes a run_ad function below, which ensures that the scenario is run with the appropriately modified adtype.
If we adopt this PR, then we have to choose between either:
Duplicating the code inside src/logdensityfunctions.jl, as I've done in this PR; or
Cutting this duplicated code out, which means that the results obtained when using this test/benchmark function will differ from the results when actually sampling a Turing model;
Removing the extra prep work inside src/logdensityfunctions.jl
(3) is a no-go as it would have noticeable impacts on performance, and even though I think it'd be very nice if we could just export a list of scenarios, I'm not really comfortable with either (1) or (2), and I don't think it's a good enough reason to do either.
The alternative to this, #882, already makes the API very straightforward (it's just one function with a very thorough docstring) and so I don't think it's unfair to define that as our interface - especially considering that it's most likely that we will actually be the ones writing the integration tests for other people.
The reasons for preference are super valid. I also think that since the hand-rolled version is not too complicated, it's worth to maintain it ourselves. Otherwise for new contributors to be able to contribute to this, they need to know what a test scenario is for DIT.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 2 of two options. The other one at #882.
Closes #869
Why am I not in favour of this one?
I think some exposition is required here, and I didn't have time to explain this super clearly during the meeting.
The API of DITest is like this:
You construct a scenario, which includes the function
f
, the value at which to evaluate it / the gradientx
, and a bunch of other things. Crucially, the scenario does not include theadtype
.You then run the scenario with an
adtype
(or an array thereof).From the perspective of generic functions
f
, this is quite a nice interface. The tricky bit with DynamicPPL, as I briefly mentioned, is that when you passLogDensityFunction
a model, varinfo, etc. it does a bunch of things that not only changes the functionf
being differentiated, but also potentially modifies theadtype
that is actually used. See, especially, this constructor:DynamicPPL.jl/src/logdensityfunction.jl
Lines 110 to 115 in 019e41b
(Note that LogDensityFunctionsAD.jl used to do this stuff for us; #806 effectively removed it and inlined its optimisations into that inner constructor.)
What this means is that, to be completely consistent with the way DynamicPPL behaves, one has to:
src/logdensityfunctions.jl
that generates the functionf
, so that the scenario can use the correctf
.adtype
, we have to make sure that scenarios generated with oneadtype
are later run with the sameadtype
.adtype
; it potentially also modifies theadtype
.make_scenario
; it also includes arun_ad
function below, which ensures that the scenario is run with the appropriately modifiedadtype
.If we adopt this PR, then we have to choose between either:
src/logdensityfunctions.jl
, as I've done in this PR; orsrc/logdensityfunctions.jl
(3) is a no-go as it would have noticeable impacts on performance, and even though I think it'd be very nice if we could just export a list of scenarios, I'm not really comfortable with either (1) or (2), and I don't think it's a good enough reason to do either.
The alternative to this, #882, already makes the API very straightforward (it's just one function with a very thorough docstring) and so I don't think it's unfair to define that as our interface - especially considering that it's most likely that we will actually be the ones writing the integration tests for other people.