-
Notifications
You must be signed in to change notification settings - Fork 5
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
Handwritten adjoints and removal of Zygote #107
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some preliminary comments on this PR. I need to write down the math to better understand what we are doing 😬
src/polar/kernels.jl
Outdated
@@ -244,6 +307,52 @@ function transfer!(polar::PolarForm, buffer::PolarNetworkState, u) | |||
wait(ev) | |||
end | |||
|
|||
KA.@kernel function adj_transfer_kernel!( | |||
adj_vmag, adj_vang, adj_pinj, adj_qinj, adj_u, pv, pq, ref, adj_pload, adj_qload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess here adj_pload
is the adjoint of the power generation pg
(adj_pg
), as the load is constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure. In that case we could remove that argument. I am not yet sure how to handle the adjoint!
signatures in general. What we also have to clarify is weither the adjoint routines return a primal value. I would not recommend that.
src/polar/kernels.jl
Outdated
@@ -130,14 +133,14 @@ KA.@kernel function residual_adj_kernel!(F, adj_F, v_m, adj_v_m, v_a, adj_v_a, | |||
if i > npv | |||
F[i + npq] -= qinj[fr] | |||
end | |||
@inbounds for c in ybus_re_colptr[fr]:ybus_re_colptr[fr+1]-1 | |||
@inbounds for c in colptr[fr]:colptr[fr+1]-1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice cleansing!
src/polar/kernels.jl
Outdated
nvbus = length(vm) | ||
nnz = length(ybus_re.nzval) | ||
T = eltype(vm) | ||
edge_vm_from = zeros(T, nnz) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if we could reuse these variables to compute the adjoint of the line flow constraints?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we could. I guess this belongs into the cache together with the adjoints.
I take shortcuts in the code; like for example when to use |
* currently implemented for `power_balance` and `reactive_power_constraints` * homogeneize signature for AutoDiff.Hessian
2b322e8
to
ba217b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok to merge once minor comments have been addressed
@@ -20,6 +20,8 @@ jobs: | |||
- uses: julia-actions/julia-runtest@latest | |||
|
|||
test-moonshot: | |||
env: | |||
CUDA_VISIBLE_DEVICES: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why setting this value? (I am curious 😺 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes the tests on moonshot run on device=1
to avoid a clash with most users due to #110.
src/Polar/Constraints/line_flow.jl
Outdated
if isa(buffer.vmag, Array) | ||
kernel! = accumulate_view!(KA.CPU()) | ||
kernel! = adj_branch_flow_kernel!(KA.CPU()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When polar
is available in the scope, I would suggest defining the kernel directly as
kernel! = adj_branch_flow_kernel!(polar.device)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh nice! indeed!
src/Polar/kernels.jl
Outdated
@@ -1,4 +1,5 @@ | |||
import KernelAbstractions: @index | |||
using ForwardDiff |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to import ForwardDiff
here, as it is already loaded in src/ExaPF.jl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok!
|
||
""" | ||
accumulate_view(x, vx, indices) | ||
# not needed in the reverse run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this dead comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to have the forward code as a comment in the adjoint function. That's just a preference, we can remove it if you don't like it. It's easier for readability of the adjoint code if someone looks at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok to keep the comment, then!
end | ||
end | ||
# not needed in the reverse run | ||
# to_flow = vmag[to_bus]^2 * ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this one?
src/Polar/kernels.jl
Outdated
# slines[ℓ + nlines] = to_flow | ||
|
||
adj_to_flow = adj_slines[ℓ + nlines] | ||
adj_vmag[to_bus] += (2 * vmag[to_bus] * ytf_abs * vmag[fr_bus]^2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would replace 2
by 2.0
(same 4
and 6
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I tried that actually yesterday to make sure that's not the cause of the numerical issues.
adj_Δθ = cosθ * adj_sinθ | ||
adj_Δθ -= sinθ * adj_cosθ | ||
adj_vang[fr_bus] += adj_Δθ | ||
adj_vang[to_bus] -= adj_Δθ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job! This adjoint does not look trivial
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why it's off!
3fb4a6d
to
84730e6
Compare
This PR introduces handwritten adjoints for all the constraints. The line flow constraint adjoints exhibit the same issues than the Zygote ones: They disagree with ForwardDiff and FD about
1e-4
. This issue has to be kept open. It is either a bug or some unknown numerical issue.