Check that if only one adjoint exists, it is not thunked

[sethaxen]

Currently, the (unenforced) standard is that single-argument functions do not thunk in the pullback. On JuliaDiff/ChainRules.jl#182, @willtebbutt and I were discussing whether we should extend that so that if there is only a single meaningful argument, then that argument's adjoint should not be thunked. This PR takes that opinion and adds a conservative test to rrule_test: namely, for functions whose pullbacks only produce one adjoint that is not DoesNotExist, that single adjoint is not thunked.

This is more to generate discussion. As you can see in the integration test, several pullbacks in ChainRules break this proposed rule:

• sum
• mean
• tr
• diag
• Symmetric
• Hermitian
• Adjoint
• adjoint
• Transpose
• transpose
• UpperTriangular
• LowerTriangular
• triu
• tril
• asum

[nickrobinson251]

I'd have thought that whether or not to thunk some computation is a question only of whether or not creating the thunk is less work then evaluating the computation in the first place, because even in the single arg case, we could encounter a * ::AbstractZero, and thanks to thunk save ever running the expensive computation.

@oxinabox ?

[willtebbutt]

Multiplication between differentials is undefined, and doesn't happen during AD. It also shouldn't happen inside pullbacks, because they operate linearly on their input (cotangent of output of primal), so there shouldn't be any internal multiply-by-AbstractZero stuff going on with differentials.

edit: if we have docs that suggest this might happen, they should be amended. I think we probably thought that this was possible before we really tied down our definition of differentials.

[oxinabox]

Zero() and (even more so) One() do still kind of serve purpose as scalars / basis elements as well as as actual differentials.
And in that role they can show up in a multiplication with a AbstractThunk and the Zero and indeed eleminate the thunk.

But even for that case, which we are not so much in-love with right now,
I don't think it is generally worth it, in the tradeoff for the extra machinery that thunking brings out.

we probably do still want to allow AbstractThunks to be the only nontrivial return so that we can still do InplaceThunk as I still haven't imagined any more efficient way to do optionally inplace accumulation.

[willtebbutt]

we probably do still want to allow AbstractThunks to be the only nontrivial return so that we can still do InplaceThunk as I still haven't imagined any more efficient way to do optionally inplace accumulation.

I think this is reasonable, but if this is the only thing that we want to do then what @sethaxen has actually implemented is fine -- it doesn't make sense for it to be a Thunk, but it does make sense for it to be an InplaceThunk.

[oxinabox]

Indeed

[willtebbutt]

Great. The question is now whether or not to view this as a breaking change. I think we probably should call it a breaking change because it will cause test failures that wouldn't previously have occurred.

The only argument in favour of non-breaking that I can think of is that anything that fails these tests would have been considered a bug previously (kind of).

So if no one objects I'm going to suggest a minor version bump.

[oxinabox]

yes, this is breaking

[willtebbutt]

LGTM, subject to minor version bump. We expect ChainRules tests to break, so CI failing for them is fine.