Simplify inverses of planar and radial layer #126
Conversation
Codecov Report
@@ Coverage Diff @@
## master #126 +/- ##
==========================================
- Coverage 57.22% 54.27% -2.95%
==========================================
Files 23 23
Lines 1169 1402 +233
==========================================
+ Hits 669 761 +92
- Misses 500 641 +141
Continue to review full report at Codecov.
|
|
The test errors are fixed in DistributionsAD 0.6.3 but it seems failing tests use DistributionsAD 0.6.2. Maybe DistributionsAD 0.6.3 is only compatible with Zygote 0.5 (Zygote is no direct dependency but implicitly some other dependencies might not be supported by Zygote < 0.5). Zygote 0.5 is only compatible with Julia >= 1.3 though. I'm wondering if we should remove CI tests on Julia < 1.3 and maybe even explicitly drop support for these versions. |
|
BTW even the latest release of Tracker requires Julia 1.3. |
There was a problem hiding this comment.
Looks good to me! Thanks @devmotion !
I've added a couple of suggest style-changes which are left-over from the old code.
Also, regarding what you're suggesting on dropping support for <1.3, I'm okay with it, but I guess this is a decision we have to make for all our packages if so.
Co-authored-by: Tor Erlend Fjelde <[email protected]>
|
For now I just removed the AD tests with Julia 1.0 (we still run the other tests with Julia 1.0 on Travis), but didn't drop the support officially. |
I think it is alright to drop support for pre-1.3 versions, as long as we keep the user informed (e.g. a warning message, if the user tries to load Turing from Julia releases under 1.3). cc @cpfiffer |
|
No clue what's going on with the Tracker tests. Maybe also caused by some package incompatibilities - it seems Bijectors pulls in FillArrays 0.9 but DistributionsAD 0.5 and 0.6 (which is only a test dependency) are not compatible with it, which in turn leads to DistributionsAD 0.1 being installed... |
|
I'm hesitant to drop support for the LTS version 1.0 before we move to the next LTS version of 1.6. It is certainly an "easy" fix, but it's not necessarily the right one -- as a major Julia package, we kind of have a responsibility to figure out how to make it work on Julia 1.0 for the moment. I think @devmotion's fix of just excluding the tests is fine -- we should also throw an error or warning whenever anyone tries to use |
Actually I don't think we have to. Major packages such as the whole DiffEq ecosystem, Tracker, and Zygote require all Julia >= 1.3 (and even the core devs mentioned at JuliaCon that it has become increasingly difficult to backport fixes to Julia 1.0). That means that basically we would have to support two completely different sets of packages, requiring different modifications and bug fixes in DistributionsAD, at the same time. I don't think it's worth the effort and time. IMO it would be much more honest to users of Julia < 1.3 to raise the compatibility bound instead of just removing the tests - then they would not get updates that are not tested on their system and probably (at least partly) don't work. |
|
BTW I just reran the tests of DistributionsAD (master branch) locally, and it seems Tracker 0.2.9 leads to test errors. So probably the Tracker-related test errors here are caused by DistributionsAD (or rather Tracker 0.2.9). |
I guess I'm alright with that fix, too. But we should make sure to be very open with everyone about what exactly this means -- any version of Turing < 1.3 will basically deteriorate into dust. I can write about it in the next release announcement. |
|
I just found an error due to Tracker 0.2.9 on a DiffEqSensitivity.jl test (downstream test from OrdinaryDiffEq.jl). It just silently computes a completely wrong value it seems: https://travis-ci.org/github/SciML/OrdinaryDiffEq.jl/jobs/717052289#L1299 . So indeed something is up there. |
|
Yeah I just opened TuringLang/DistributionsAD.jl#107 |
This reverts commit 1ad8368.
|
Tracker 0.2.10 should fix the problems that we observed. |
|
Sweet! So this is good-to-merge after approval then? |
|
We still have to decide what to do about Julia < 1.3. The tests will still fail with Julia 1.0 |
|
Ah, aight 👍 |
|
So what should we do? I'd suggest to require Julia >= 1.3 (also in DistributionsAD, in fact we just didn't test Julia 1.0 there) and then test Julia 1.3 as well to ensure it actually works properly. |
|
I'm okay with that suggestion. |
|
Probably best to wait a bit before merging, so people can comment on https://discourse.julialang.org/t/rfc-dropping-support-for-julia-1-3-in-turing-jl/45015. |
|
The latest release of Distributions pulls in FillArrays 0.9 for which no compatible Zygote version exists yet. |
|
Would be fixed by FluxML/Zygote.jl#768. |
| @@ -20,14 +20,14 @@ StatsFuns = "4c63d2b9-4356-54db-8cca-17b64c39e42c" | |||
| [compat] | |||
| ArgCheck = "1, 2" | |||
| Compat = "3" | |||
| Distributions = "0.23.3" | |||
| Distributions = "=0.23.3, =0.23.4, =0.23.5, =0.23.6, =0.23.7, =0.23.8" | |||
There was a problem hiding this comment.
Avoids pulling in FillArrays 0.9. The proper fix is rather to adjust Zygote's compatibilities, but I'm not sure how long it takes for the Zygote PR to be released. In the meantime this should avoid the current test failures.
Would be good to fix this before the next release to avoid some weird compatibilities ending up in the registry (which might lead to outdated versions of Bijectors).
This PR simplifies the computation of the inverses of the planar and radial layer by avoiding redundant and unnecessary computations + providing an initial bracketing interval (planar layer) and computing the inverse in closed form (radial layer).
Additionally, this PR fixes #124.