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

Add rules for sqrt(::Diagonal) #509

Merged
merged 8 commits into from
Aug 24, 2021
Merged

Add rules for sqrt(::Diagonal) #509

merged 8 commits into from
Aug 24, 2021

Conversation

thomasgudjonwright
Copy link
Contributor

@thomasgudjonwright thomasgudjonwright commented Aug 23, 2021

Adding an rrule for sqrt(::Diagonal), relating to a comment in an issue discussing structural vs natural Tangents

@mzgubic
Copy link
Member

mzgubic commented Aug 23, 2021

The issue is that the rand_tangent treats Integers as non differentiable, i.e.

julia> rand_tangent(Diagonal([1,2, 3.0]))
3×3 Diagonal{Float64, Vector{Float64}}:
 -2.21         
       2.4     
           -8.98

julia> rand_tangent(Diagonal([1,2,3]))
NoTangent()

Project.toml Outdated Show resolved Hide resolved
test/rulesets/LinearAlgebra/structured.jl Outdated Show resolved Hide resolved
test/rulesets/LinearAlgebra/structured.jl Outdated Show resolved Hide resolved
src/rulesets/LinearAlgebra/structured.jl Outdated Show resolved Hide resolved
src/rulesets/LinearAlgebra/structured.jl Outdated Show resolved Hide resolved
@thomasgudjonwright thomasgudjonwright changed the title WIP: add diag sqrt rule Add rules for sqrt(::Diagonal) Aug 23, 2021
@mzgubic
Copy link
Member

mzgubic commented Aug 24, 2021

@mcabbott are you happy to approve the current state? I pushed commits so it's good if you approve as well. We couldn't quite figure out what went wrong with Tom's local setup but everything works when he does a fresh git clone

@mzgubic
Copy link
Member

mzgubic commented Aug 24, 2021

LOL, okay, so after pushing the rand(3) to the tests they failed on nightly as well. I think it's that FiniteDifferences is doing some sort of step finding and accidentally calls sqrt of a negative number. Multiplying the rand() by 10 worked locally, but it's probably better to just fix them to some fixed matrix to avoid stochastic failures.

The weird part is that I could only reproduce locally by running the test suite (and not copy pasting the tests) so it looks like this was some kind of interaction with the JuliaInterpretter

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