Allow to sum NotImplemented

[devmotion]

This PR fixes the test failures in JuliaDiff/ChainRulesTestUtils.jl#140.

Alternatively, one could This PR allows +(::NotImplemented, y) (and permutations) more generally.

[mzgubic]

My preference would be to go with +, thoughts @oxinabox ?

[oxinabox]

I do rather overload +, seems simpler.

Downside is it would make it possible to add primal + NotImplemented()
Which we don't really want.
But might tolerate, it's not the worst.
(One solution to that might be to seperate + (for two differetials) from exponential map (weird name but it means primal + differetial), but that is a much bigger change).

Compromise could be to only all + to be propagating between NotImplented and AbstractDifferential types.

I suspect we also should overload getproperty on NotImplented to return NotImplented.
(Again like AbstractZeros do)

[devmotion]

Compromise could be to only all + to be propagating between NotImplented and AbstractDifferential types.

I tried this first but unfortunately it is not sufficient. The newly introduced tests in JuliaDiff/ChainRulesTestUtils.jl#140 still fail: the add!! check tries to sum two Composites of which one has a field of type NotImplemented, and the other one a corresponding field of type Float64. So if elementwise_add is not modified, it is necessary to define +(::NotImplemented, ::Float64).

[oxinabox]

Hmm let's get some more opinions.

@willtebbutt @sethaxen

Should we make + with NotImplented fully propagating the NotImplented (and never erroring).
which us simpler than this PR, and I suspect we will need to do anyway at some point for other reasons.
But which has the downside of no built in protection against adding to it's primal value. Though you will get it eventually since it propagates out to the final answer.

Or do we just allow that for "fields" of a Composite (this PR).

[sethaxen]

I am unlikely to have time to really dig into this PR, and I have not been tracking the recent changes to NotImplemented. My understanding is that currently, NotImplemented stores the line where it was raised along with a message. This information seems vital, and it seems like any change that would lose this information would be bad. So adding NotImplemented to a primal does not seem bad. Adding two NotImplemented is not ideal because one of the messages would be lost, but as long as a message and LOC is given eventually, this is not terrible. My hot take is that it would be cleaner to handle this with overloading + than to special-case Composite fields.

[willtebbutt]

I don't have any particular objection to implementing + -- seems reasonable to me. We can always revert and reconsider if it starts to break everything 🤷

[devmotion]

I updated the PR and enabled + for NotImplemented globally. I did not remove the tests of NotImplemented and Composite since they cover the problem observed in ChainRulesTestUtils.

[mzgubic]

Thanks for fixing this, looks good to me.

[mzgubic]

We probably don't need to test exhaustively? IMO we could just test Composite{Foo} with (a, b), (b, a) here.

[devmotion]

(b, b) tests +(::NotImplemented, ::NotImplemented) but, as the other two cases, in principle this is covered by the NotImplemented tests now as well. I kept the tests of the original PR since this was the actual problem that caused errors in ChainRulesTestUtils. Would you like me to remove the Composite tests?

[mzgubic]

That would be my preference, but it is not strong and I leave it up to you to make a decision. Feel free to merge as is!

[devmotion]

If it's not necessary to remove the tests, I would like to keep them since they check the actual problem that this PR tries to solve. I assume that this is an advantage if, e.g., at some point a more restrictive policy regarding summation of NotImplemented is adopted.