ForwardDiff -> 1 #871
ForwardDiff -> 1 #871
Conversation
Benchmark Report for Commit ac22eb8Computer InformationBenchmark Results |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #871 +/- ##
=======================================
Coverage 82.92% 82.92%
=======================================
Files 36 36
Lines 3964 3964
=======================================
Hits 3287 3287
Misses 677 677 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Pull Request Test Coverage Report for Build 14134335858Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 15324866261Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 14134335858Details
💛 - Coveralls |
|
|
penelopeysm
force-pushed
the
py/forwarddiff-1
branch
from
7f4e5d0 to
d0bb1fb
Compare
|
DynamicPPL.jl documentation for PR #871 is available at: |
| @testset "ad.jl" begin | ||
| @testset "logp" begin | ||
| # Hand-written log probabilities for vector `x = [s, m]`. | ||
| function logp_gdemo_default(x) | ||
| s = x[1] | ||
| m = x[2] | ||
| dist = Normal(m, sqrt(s)) | ||
|
|
||
| return logpdf(InverseGamma(2, 3), s) + | ||
| logpdf(Normal(0, sqrt(s)), m) + | ||
| logpdf(dist, 1.5) + | ||
| logpdf(dist, 2.0) | ||
| end | ||
|
|
||
| test_model_ad(gdemo_default, logp_gdemo_default) | ||
|
|
||
| @model function wishart_ad() | ||
| return v ~ Wishart(7, [1 0.5; 0.5 1]) | ||
| end | ||
|
|
||
| # Hand-written log probabilities for `x = [v]`. | ||
| function logp_wishart_ad(x) | ||
| dist = Wishart(7, [1 0.5; 0.5 1]) | ||
| return logpdf(dist, reshape(x, 2, 2)) | ||
| end | ||
|
|
||
| test_model_ad(wishart_ad(), logp_wishart_ad) | ||
| end | ||
| end |
There was a problem hiding this comment.
For full transparency, part of the motivation for deleting this file is that it's a bit pointless given that we have quite extensive AD testing now, but part of it is also because ForwardDiff@1 broke this test.
Specifically, the change to Dual equality in JuliaDiff/ForwardDiff.jl#481 means that FD cannot differentiate through cholesky -- already reported here: JuliaDiff/ForwardDiff.jl#606 and thus also logp of Wishart.
(For context, that ForwardDiff PR was merged in a 0.10 version, then yanked, and rereleased in 1.0, which is why it's coming back again in this PR)
This bug is trivial to reproduce with just ForwardDiff + Distributions:
using ForwardDiff, Distributions
d = Wishart(7, [1 0.5; 0.5 1])
x = rand(d)
function logp(x)
return logpdf(d, reshape(x, 2, 2))
end
ForwardDiff.gradient(logp, x)and thus has nothing to do with DynamicPPL, and I don't consider it our responsibility to work around it in our test suite.
Given all of the above, I don't mind just deleting this test.
There was a problem hiding this comment.
BTW, it still works for linked varinfo so in practice won't be a problem because AD only really runs on linked varinfo:
# make sure to use DynamicPPL with this PR and ForwardDiff@1
using DynamicPPL, Distributions, ForwardDiff, ADTypes
using DynamicPPL.TestUtils.AD: run_ad
@model f() = x ~ Wishart(7, [1 0.5; 0.5 1])
model = f()
unlinked_vi = VarInfo(model)
linked_vi = DynamicPPL.link(unlinked_vi, model)
run_ad(f(), AutoForwardDiff(); varinfo=unlinked_vi) # errors
run_ad(f(), AutoForwardDiff(); varinfo=linked_vi) # fine
There was a problem hiding this comment.
relieving to know it works with linked VarInfo
|
I know the answer probably is "no", but do we want to update the version of benchmark https://github.com/TuringLang/DynamicPPL.jl/blob/py/forwarddiff-1/benchmarks/Project.toml ? |
Oh, good spot! I totally forgot about that because CompatHelper didn't open a PR for it. Thanks thanks. Let me fix that. |
Closes #864
Closes #874
Closes #876