Revert "Proper support for distributions with embedded support" #486
Conversation
This reverts commit 7b01d25.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -206,8 +206,7 @@ DynamicPPL.link!! | |||
| DynamicPPL.invlink!! | |||
| DynamicPPL.default_transformation | |||
| DynamicPPL.maybe_invlink_before_eval!! | |||
| DynamicPPL.reconstruct | |||
| ``` | |||
| ``` | |||
There was a problem hiding this comment.
[JuliaFormatter] reported by reviewdog 🐶
| ``` | |
| ``` |
|
Before we revert this change: the only test that is failing is a Tracker-related one 😬 I'm preferential to just marking this as broken for now. |
|
Do we test ReverseDiff as well? I would expect that it is broken as well since the issue looks to me like the FillArrays problem. It seems we run tests with FillArrays 1.2.0, so fixing the FillArrays compat (TuringLang/DistributionsAD.jl#248) might be sufficient I assume. |
Pull Request Test Coverage Report for Build 5314270448
💛 - Coveralls |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #486 +/- ##
==========================================
- Coverage 75.71% 74.20% -1.51%
==========================================
Files 21 21
Lines 2487 2466 -21
==========================================
- Hits 1883 1830 -53
- Misses 604 636 +32
☔ View full report in Codecov by Sentry. |
This is the integrations tests with Turing, so all backends should be tested. |
|
Based on the log (https://github.com/TuringLang/DynamicPPL.jl/actions/runs/5278661944/jobs/9548298259#step:6:1893) the test issue is in a separate test outside of the AD loop: https://github.com/TuringLang/Turing.jl/blob/6a307cd31ce62667773dc7958927255bdb4699c8/test/essential/ad.jl#L99 There ReverseDiff is explicitly disabled since apparently it already failed previously. (And the AD loop only includes ForwardDiff and ReverseDiff?) |
|
Oooooo I didn't think of the fact that it might be disabled in Turing. My bad! |
|
But yeah, then you're probably right that it's because of the FillArrays stuff. |
|
But have you seen the perf-regressions in the DistributionsAD.jl PR you linked? ForwardDiff takes 220mins to run the tests.. Uncertain if that's what we want to do 😕 |
But is that new? I had seen extreme ForwardDiff times before, it was usually massively slow IIRC. |
I'll double check! |
|
Nope, you're right! Also incrediblly slow on master 👍 I thought I experienced differently on the other PR but must have just been my perception. EDIT: Maybe the 👍 isn't quite the right sentiment 🙃 |
Looking at this though, it's not obvious to me how this is related to FillArrays? Moreover, the example in there runs perfectly on my end 😬 EDIT: Nvm, wait. I wasn't running using the correct version! |
|
Nah, it's actually working just fine: julia> Turing.setadbackend(:tracker)
┌ Warning: Usage of Tracker.jl with Turing.jl is no longer being actively tested and maintained; please use at your own risk. See Zygote.jl or ReverseDiff.jl for fully supported reverse-mode backends.
└ @ Turing.Essential ~/.julia/packages/Turing/cplDJ/src/essential/ad.jl:15
^P^P:tracker
julia> sample(invwishart(), HMC(0.01, 1), 1000);
Sampling 100%|██████████████████████████████████████████████████████████████████████████████████████| Time: 0:00:07
(jl_8mpQcR) pkg> st
Status `/tmp/jl_8mpQcR/Project.toml`
[366bfd00] DynamicPPL v0.23.0 `https://github.com/TuringLang/DynamicPPL.jl.git#master`
[fce5fe82] Turing v0.26.0 |
|
What exactly? I can reproduce the error in a clean environment by running julia> ] add DynamicPPL#master Turing
julia> using Turing, LinearAlgebra
julia> @model function wishart()
theta ~ Wishart(4, Matrix{Float64}(I, 4, 4))
end
wishart (generic function with 2 methods)
julia> Turing.setadbackend(:tracker)
┌ Warning: Usage of Tracker.jl with Turing.jl is no longer being actively tested and
│ maintained; please use at your own risk. See Zygote.jl or ReverseDiff.jl for fully
│ supported reverse-mode backends.
└ @ Turing.Essential /home/david/.julia/packages/Turing/cplDJ/src/essential/ad.jl:15
:tracker
julia> sample(wishart(), HMC(0.01, 1), 1000);
Sampling 100%|█████████████████████████████████████████████████████| Time: 0:00:06
ERROR: MethodError: no method matching Float64(::Tracker.TrackedReal{Float64})
Closest candidates are:
(::Type{T})(::Real, ::RoundingMode) where T<:AbstractFloat
@ Base rounding.jl:207
(::Type{T})(::T) where T<:Number
@ Core boot.jl:792
(::Type{T})(::AbstractChar) where T<:Union{AbstractChar, Number}
@ Base char.jl:50
...
Stacktrace:
[1] convert(#unused#::Type{Float64}, x::Tracker.TrackedReal{Float64})
@ Base ./number.jl:7
...The output of |
|
Wat.. (jl_8mpQcR) pkg> st
Status `/tmp/jl_8mpQcR/Project.toml`
[366bfd00] DynamicPPL v0.23.0 `https://github.com/TuringLang/DynamicPPL.jl.git#master`
[fce5fe82] Turing v0.26.0
julia> versioninfo()
Julia Version 1.9.0
Commit 8e630552924 (2023-05-07 11:25 UTC)
Platform Info:
OS: Linux (x86_64-linux-gnu)
CPU: 12 × Intel(R) Core(TM) i7-10710U CPU @ 1.10GHz
WORD_SIZE: 64
LIBM: libopenlibm
LLVM: libLLVM-14.0.6 (ORCJIT, skylake)
Threads: 1 on 12 virtual cores
julia> using Turing
julia> @model function invwishart()
theta ~ InverseWishart(4, Matrix{Float64}(I, 4, 4))
end
invwishart (generic function with 2 methods)
julia> using LinearAlgebra
julia> Turing.setadbackend(:tracker)
┌ Warning: Usage of Tracker.jl with Turing.jl is no longer being actively tested and maintained; please use at your own risk. See Zygote.jl or ReverseDiff.jl for fully supported reverse-mode backends.
└ @ Turing.Essential ~/.julia/packages/Turing/cplDJ/src/essential/ad.jl:15
:tracker
julia> sample(invwishart(), HMC(0.01, 1), 1000);
Sampling 100%|██████████████████████████████████████████████████████████████████████████████████████| Time: 0:00:00
|
|
AAAAAAh |
|
I realized that we might not want to incorporate changes from the master branch. Feel free to undo my commit if needed. |
|
I think this is not needed anymore since Turing fixed FillArrays to 1.0.0. |
Reverts #462
Since Turing tests fail.