Skip to content

Accumulate derivative into Adjoint's original elements #184

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Accumulate derivative into Adjoint's original elements #184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
95c91ce
accumulate derivative into Adjoint's original elements
Sep 15, 2021
de22270
fix jaconbian: LA.lu on Adjoint
Sep 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/api/tape.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,8 @@ end
# JacobianTape #
################

LinearAlgebra.lu(x::Adjoint, args...) = LinearAlgebra.lu(Array(x), args...)
Copy link
Member

Choose a reason for hiding this comment

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

This is a quite terrible type piracy 😥

Copy link
Contributor

@yebai yebai Sep 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

would it be possible to definite a lu(x::TrackedArray{Adjoint,..}, ...) instead here to avoid/reduce type piracy?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure, since this is so unrelated to the other changes to me it seems generally a bit weird. Why do we have to "fix" lu if the problem is the accumulation of the derivatives? I understand (or assume) it is necessary to fix some test errors but I think this points to another deeper problem or at least requires a more general solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the introduction of adjoint in DiffRules also causes failure of another test case which is unrelative to broadcasting on Adjoint:
https://github.com/JuliaDiff/ReverseDiff.jl/blob/master/test/api/JacobianTests.jl#L293

My intention was indeed to fix it, but I am not familiar with LA and Jacobian Matrix, I just went through the error log and found it needs such a method. Sorry about that. I hope somebody would like to dig deeper into this. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

No worries 🙂 I just found #183 (comment), so there's definitely a lot of not nice/non-general code regarding Adjoint already in ReverseDiff 🙈 Would make it even nicer to fix these problems more generally.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is functionally correct, shall we 1) open a new issue 2) leave more improvements as separate PR?


"""
ReverseDiff.JacobianTape(f, input, cfg::JacobianConfig = JacobianConfig(input))

Expand Down
4 changes: 4 additions & 0 deletions src/derivatives/propagation.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ index_bound(x::AbstractArray, ::AbstractArray{T,N}) where {T,N} = CartesianIndex
@inline increment_deriv!(t::TrackedArray, x::Real, i) = (t.deriv[i] += x; nothing)
@inline increment_deriv!(t::AbstractArray, x::AbstractArray, i) = increment_deriv!(t[i], x[i])
@inline increment_deriv!(t::AbstractArray, x::Real, i) = increment_deriv!(t[i], x)
@inline increment_deriv!(t::Adjoint, x::AbstractArray, i) = increment_deriv!(t.parent[LinearIndices(t.parent)'[i]], x[i])
@inline increment_deriv!(t::Adjoint, x::Real, i) = increment_deriv!(t.parent[LinearIndices(t.parent)'[i]], x)

function increment_deriv!(t::AbstractArray, x)
for i in eachindex(t)
Expand All @@ -57,6 +59,8 @@ end
@inline decrement_deriv!(t::TrackedArray, x::Real, i) = (t.deriv[i] -= x; nothing)
@inline decrement_deriv!(t::AbstractArray, x::AbstractArray, i) = decrement_deriv!(t[i], x[i])
@inline decrement_deriv!(t::AbstractArray, x::Real, i) = decrement_deriv!(t[i], x)
@inline decrement_deriv!(t::Adjoint, x::AbstractArray, i) = decrement_deriv!(t.parent[LinearIndices(t.parent)'[i]], x[i])
@inline decrement_deriv!(t::Adjoint, x::Real, i) = decrement_deriv!(t.parent[LinearIndices(t.parent)'[i]], x)

function decrement_deriv!(t::AbstractArray, x)
for i in eachindex(t)
Expand Down
20 changes: 10 additions & 10 deletions test/derivatives/LinAlgTests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ function test_arr2arr(f, a, b, tp)
ReverseDiff.value!(at, a2)
ReverseDiff.forward_pass!(tp)
@test value(ct) == f(a2, b)

ReverseDiff.value!(at, a)
empty!(tp)

Expand Down Expand Up @@ -207,17 +207,17 @@ function test_arr2arr_inplace(f!, f, c, a, b, tp)
empty!(tp)
end

for f in (
sum,
det,
mean,
y -> dot(vec(y), vec(y)),
y -> vec(y)' * vec(y),
y -> vec(y)' * ones(length(y)),
y -> ones(length(y))' * vec(y),
for (f, no_tape_len) in (
(sum, false),
(det, false),
(mean, false),
(y -> dot(vec(y), vec(y)), false),
(y -> vec(y)' * vec(y), true),
(y -> vec(y)' * ones(length(y)), true),
(y -> ones(length(y))' * vec(y), true),
)
test_println("Array -> Number functions", f)
test_arr2num(f, x, tp)
test_arr2num(f, x, tp, ignore_tape_length=no_tape_len)
end

for f in (
Expand Down