Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

basic sparse handling #762

Closed
wants to merge 19 commits into from
Closed

basic sparse handling #762

wants to merge 19 commits into from

Conversation

DhairyaLGandhi
Copy link
Member

@willtebbutt
Copy link
Member

Can this be closed in favour of JuliaDiff/ChainRules.jl#246?

test/gradcheck.jl Outdated Show resolved Hide resolved
Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of comments.

src/lib/array.jl Outdated Show resolved Hide resolved
src/lib/array.jl Outdated Show resolved Hide resolved
@DhairyaLGandhi
Copy link
Member Author

bors r+

Let's also add the corresponding changes in ChainRules, but I think we have some work looking to use sparse handling currently, and we can dd it to the list of things we have to move.

bors bot added a commit that referenced this pull request Dec 23, 2020
762: basic sparse handling r=DhairyaLGandhi a=DhairyaLGandhi

Relates to #570, SciML/DifferentialEquations.jl#649, #742

Co-authored-by: Dhairya Gandhi <dhairya@juliacopmuting.com>
Co-authored-by: Dhairya Gandhi <dhairya@juliacomputing.com>
@DhairyaLGandhi
Copy link
Member Author

bors r-

Reviews 👍

@bors
Copy link
Contributor

bors bot commented Dec 23, 2020

Canceled.

src/lib/array.jl Outdated Show resolved Hide resolved
Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just needs a patch bump :)

test/gradcheck.jl Outdated Show resolved Hide resolved
src/lib/array.jl Outdated Show resolved Hide resolved
@ChrisRackauckas
Copy link
Member

That small difference would cause weird issues with identity.(A) and such, but also f.(A) would do Float32.(A) instead of f.(A) which would lead to incorrect results, so that might be what's causing gradient issues.

Oof, thanks!

Co-authored-by: Christopher Rackauckas <accounts@chrisrackauckas.com>
@test gradtest(x -> sum(sparse(x)), rand(Float32, 3,3))
@test gradtest(x -> sum(sparse(x)), rand(Float32, 3)) # test vectors also
@test gradcheck(x -> sum(diagm(x)), sparse(rand(3)))
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can add a test for broadcasting

src/lib/array.jl Outdated Show resolved Hide resolved
@@ -2,6 +2,8 @@ using Zygote, Test, Random, LinearAlgebra, Statistics, FillArrays,
AbstractFFTs, FFTW, Distances
using Zygote: gradient
using Base.Broadcast: broadcast_shape
using Distributed: pmap
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this import?

@CarloLucibello
Copy link
Member

a test is failing

@CarloLucibello
Copy link
Member

Actually, I see you also filed JuliaDiff/ChainRules.jl#246, can we close this PR in favor of that?

@CarloLucibello
Copy link
Member

Closing since this has been superseded by JuliaDiff/ChainRules.jl#579

@ChrisRackauckas ChrisRackauckas deleted the dg/sparse branch June 11, 2022 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants