Simplify inverses of planar and radial layer

[devmotion]

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 https://github.com/TuringLang/Bijectors.jl/issues/124.

[codecov[bot]]

Codecov Report

Merging #126 into master will decrease coverage by 2.94%. The diff coverage is 95.00%.

@@            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     

Impacted Files	Coverage Δ
src/compat/tracker.jl	45.45% <60.00%> (-10.11%)	:arrow_down:
src/bijectors/planar_layer.jl	100.00% <100.00%> (+10.25%)	:arrow_up:
src/bijectors/radial_layer.jl	100.00% <100.00%> (ø)
src/bijectors/normalise.jl	75.00% <0.00%> (-18.11%)	:arrow_down:
src/bijectors/adbijector.jl	71.42% <0.00%> (-11.91%)	:arrow_down:
src/bijectors/truncated.jl	48.19% <0.00%> (-5.15%)	:arrow_down:
src/transformed_distribution.jl	53.77% <0.00%> (-4.66%)	:arrow_down:
src/bijectors/pd.jl	48.00% <0.00%> (-4.18%)	:arrow_down:
src/bijectors/stacked.jl	91.37% <0.00%> (-2.86%)	:arrow_down:
... and 11 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 5fd780c...8bccae4. Read the comment docs.

[devmotion]

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.

[devmotion]

BTW even the latest release of Tracker requires Julia 1.3.

[devmotion]

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.

[yebai]

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.

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

[devmotion]

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...

[cpfiffer]

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 Zygote or Tracker on Julia < 1.3.

[devmotion]

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.

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.

[devmotion]

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).

[cpfiffer]

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.

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.

[ChrisRackauckas]

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.

[devmotion]

Yeah I just opened https://github.com/TuringLang/DistributionsAD.jl/issues/107

[devmotion]

Tracker 0.2.10 should fix the problems that we observed.

[torfjelde]

Sweet! So this is good-to-merge after approval then?

[devmotion]

We still have to decide what to do about Julia < 1.3. The tests will still fail with Julia 1.0

[torfjelde]

Ah, aight :+1:

[devmotion]

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.

[cpfiffer]

I'm okay with that suggestion.

[devmotion]

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.

[devmotion]

The latest release of Distributions pulls in FillArrays 0.9 for which no compatible Zygote version exists yet.

[devmotion]

Would be fixed by https://github.com/FluxML/Zygote.jl/pull/768.