Fix rrules for Symmetric and Diagonal constructors

[ararslan]

Currently these definitions are extending rrule(::typeof(T), x) where T is a type. However, typeof(Diagonal) == UnionAll, which means this is not defining the method it looks like it might be defining. The only reason this worked when originally implemented was that one of the rrule definitions was for rrule(UnionAll, Matrix) and the other for rrule(UnionAll, Vector), so dispatch still worked.

This replaces these problematic ::typeof(T)s with ::Type{<:T}.

[willtebbutt]

Great job @ararslan . Could you please add a test which would have broken the old implementation before merging though, just to prevent accidental reversion at some point?

[ararslan]

I had thought of doing so but wasn't able to determine how to formulate such a test. The only way I can think to properly catch this case would be to add more matrix constructor sensitivities, which would show method overwriting warnings for rrule(::Type{UnionAll}, ::Matrix). Any suggestions?

[willtebbutt]

I was just thinking something simple like checking that

rrule(Diagonal{Float64}, x)
rrule(Diagonal, x)

should do sensible things. Wouldn't generalise to new types of course, but would at least prevent these particular ones from reverting.

Update: perhaps update the rrule_test invocations in the existing tests. I think we'll just have to be vigilant moving forwards to make sure that this stuff is tested appropriately on a case-by-case basis.

[ararslan]

perhaps update the rrule_test invocations in the existing tests

Update them in what way?

[willtebbutt]

Update them in what way?

Sorry, I was just thinking something along the lines of changing this and related to

rrule_test(typeof(Diagonal(randn(rng, N))), randn(rng, N, N), (randn(rng, N), randn(rng, N)))

[ararslan]

Okay, I've implemented that for Diagonal, but Symmetric is considerably more difficult, as no one-argument constructor exists for Symmetric concrete subtypes.

[willtebbutt]

but Symmetric is considerably more difficult, as no one-argument constructor exists for Symmetric concrete subtypes.

Is Symmetric(randn(N, N)) not viable?

[ararslan]

Well it is, but that's covered by the existing Symmetric tests. If you look at my recent addition to the Diagonal tests, you'll see that you can pass the concrete type easily as a constructor to rrule_test, and all is well. But rrule_test calls f(x), and no such method exists if f is a concrete subtype of Symmetric; it requires a second argument specifying which is the triangle to consider "primary."

[willtebbutt]

Ahh I see what you mean. Good point -- we should probably sort this out in another PR as we'll probably have more of these things to check in the future. Would you mind raising an issue regarding this?

I'm happy with the PR now though. Thanks for sorting this out.

[ararslan]

Issue in #24.