Add basic sparse handling

[DhairyaLGandhi]

Questions:

• how do I test things?
• Will this be the right test here?
• Also this adds an rrule for a constructor, is that a problem?

[willtebbutt]

how do I test things?

See other examples in this package, e.g. here. ChainRulesTestUtils.jl should always be used, unless it's actually not possible for some reason.

Will this be the right test here?

Not clear what you mean by this. Could you clarify please?

Also this adds an rrule for a constructor, is that a problem?

rrules for constructors are fine :)

[DhairyaLGandhi]

Not clear what you mean by this. Could you clarify please?
I feel like I'm using the API incorrectly, but none seemed to work

rrule_test(x -> sum(sparse(x)), rand(), (rand(3,3), rand()))

Should have, I thought, but maybe you could clarify

[willtebbutt]

rrule_test(x -> sum(sparse(x)), rand(), (rand(3,3), rand()))

Should have, I thought, but maybe you could clarify

The first element of the last argument is the value at which you want to evaluate the rrule, call it x. That looks like it's fine in your case. The second element should be an appropriate differential for x, call it dx. Since x is a Matrix{Float64}, dx should be a Matrix{Float64} of the same size.

As regards the first argument, rrule_test literally only handles rrules, and can't handle their composition with other things. Consequently, if you want to test sparse, you should only pass in sparse. The reason it can't handle composition with other functions is because you would need an AD to do this, and ChainRules is AD-agnostic. Call the first argument f.

Finally, the second argument has to be an appropriate differential for f(x). In this case, this means that you need to define what it means to be an appropriate differential for a SparseMatrixCSC{Float64,Int64}. I'm guessing that some arbitrary AbstractMatrix{Float64} is not generally appropriate because it will destroy the complexity guarantees associated with sparse arrays.

On that note, it might be worth reading #232 if you've not already -- there's a large discussion going on about the "right" way to think about this, which might be helpful. It would be great if you could chime in with your thoughts on the matter.

[DhairyaLGandhi]

The reason it can't handle composition with other functions is because you would need an AD to do this, and ChainRules is AD-agnostic

That would explain why sparse wouldn't work either. It would need to test SparseMatrixCSC directly, since there is no AD recursion into the stacks happening either. Thanks for pointing me to the issue, I do think having a robust testing pipeline is necessary for the different levels of abstractions, and this makes sense for ChainRules.

[willtebbutt]

That would explain why sparse wouldn't work either. It would need to test SparseMatrixCSC directly, since there is no AD recursion into the stacks happening either.

That's not quite true. IIUC you've implemented an rrule for sparse directly, so you should be able to use CRTU (ChainRulesTestUtils) on it. The question is just what the differential of the output should be.

Thanks for pointing me to the issue, I do think having a robust testing pipeline is necessary for the different levels of abstractions, and this makes sense for ChainRules.

Agreed. The issue in question has signficant ramifications for Zygote as well though -- there are currently loads of rules implemented there that are causing the kinds of problems discussed in that issue. But that's a separate issue.

[nickrobinson251]

thanks for the contribution!

I think @willtebbutt is helping out with testing. i've left some on the src 👍

[nickrobinson251]

please can you use 4-space indenting throughout, as per the style guide? https://github.com/invenia/BlueStyle#synopsis :)

[sethaxen]

I have a current project for which I need some rrules for SparseMatrixCSC. @DhairyaLGandhi are you planning to work on this in the near future? And if not, would you mind if I continued this work (in a new PR)?

[DhairyaLGandhi]

Sure, please go ahead! Apologies I missed this comment in the GitHub blackhole. I need this for a project as well, so would love to get this through.

[willtebbutt]

@sethaxen please ping me when ready for review.

[oxinabox]

Not a full review, but just so it doesn't get missed, it is the pattern of this package, inline with BlueStyle,
to do all the usings at the top level file

[CarloLucibello]

Just hit this problem in CarloLucibello/GraphNeuralNetworks.jl#113.
Is there any interest in finishing up this PR soon (it has been open for a year and a half)? Otherwise, I'll create a new one

[sethaxen]

@CarloLucibello i won't be able to work on this soon. Feel free to take over!

[CarloLucibello]

I think this can be closed now that #579 is merged