Simplify implementation and tests in #1534

[devmotion]

@matbesancon This PR simplifies the implementation and tests in #1534. It also makes the code a bit more consistent with the conventions in ChainRules, fixes some type stability issue, and simplifies the handling of non-finite values in _logpdf.

Tests pass locally with Julia 1.7 and JuliaDiff/ChainRulesTestUtils.jl#247.

Edit: The CRTU PR was merged and released.

[devmotion]

@matbesancon Tests all pass it seems, with Julia 1.3, 1.7, and nightly.

[AlexRobson]

fwiw, running something completely different these tests were failing in 1.6, though all passing in 1.8:

In 1.6:

Got exception outside of a @test
  DomainError with [0.100746, 0.151104, 0.198606, 0.106677, 0.417981, 0.234742, 0.910606, 0.501805, 0.861964, -0.00290635]:
  Dirichlet: alpha must be a positive vector.

Is this an RNG issue, or something tied up with the julia version?

If this diagnosis is correct, assuming it's the former, and if chainrules are going to start being added here, It may be worth specifying the tangents in the the test_rrule to avoid RNG issues. It looks like a lot of the testsets are seeded here, so I think the equivalent would be:

Random.MersenneTwister(1)
test_rrule(Distributions._logpdf, d, x ⊢ CRTU.rand_tangent(x); fdm=fdm, rtol=1e-5, nans=true)

[devmotion]

Since tests pass (see e.g. #1606) I don't see any reason to tweak seeds right now. If it becomes necessary, we could possibly specify a seed but that is also not guaranteed to yield the same random numbers on different Julia versions (and OS IIRC). (BTW @testset already resets the seed, so it might not be necessary to specify a seed for every test but could be done once initially: https://docs.julialang.org/en/v1/stdlib/Test/#Test.@testset) Generally, I don't think we want to specify tangents manually if it can be avoided - specific choices make it easier to miss broken cases, and I feel like by the test_rrule API it's intended to provide tangents manually only if specific choices are needed.

[devmotion]

Is this an RNG issue, or something tied up with the julia version?

Possibly finite differencing fails for some unfortunate cases. We could maybe also try to pick values that are further away from zero or tweak the finite differencing method.

[AlexRobson]

Since tests pass (see e.g. #1606) I don't see any reason to tweak seeds right now. If it becomes necessary, we could possibly specify a seed but that is also not guaranteed to yield the same random numbers on different Julia versions (and OS IIRC). (BTW @testset already resets the seed, so it might not be necessary to specify a seed for every test but could be done once initially: https://docs.julialang.org/en/v1/stdlib/Test/#Test.@testset) Generally, I don't think we want to specify tangents manually if it can be avoided - specific choices make it easier to miss broken cases, and I feel like by the test_rrule API it's intended to provide tangents manually only if specific choices are needed.

Good points. I was considering the cases where adding in other unrelated testsets can change the RNG and I'd actually forgotten testset resets the seed, so this is essentially equivalent. For version changes, technically StableRNGs.jl could be used, but for here I also now don't see any particular reason to go out of the way to tweak. I just happened to be using the wrong version of julia when playing around with what I was working on when testing :)