-
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
Hessian adjoint tangent projection using ForwardDiff over adjoint residual function #99
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.
I like 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.
For some reason AutoDiff.residual_hessian!
is allocating a lot on large cases. This should not be the case. I am currently investigating that.
src/autodiff.jl
Outdated
else | ||
error("Unsupported Jacobian structure") | ||
end | ||
uncompress_kernel!(arrays.J, arrays.compressedJ, arrays.coloring) |
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.
We have an issue here: the type of arrays.J
is inferred as SparseMatrixCSC
, instead of SparseMatrixCSC{Float64, Int}
. That leads to some inefficiencies in uncompress_kernel!
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.
In fact, on line 80 of src/autodiff.jl
, we should replace:
SMT = SparseMatrixCSC
```julia
by
```julia
SMT = SparseMatrixCSC{Float64, Int}
(yeah, type inference is really my new dada)
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.
Done.
Sorry @frapac, a lot rewiring. In the end it's much less code. I removed everything we don't need. That's also why I might want to keep the history this time. In case I want to do that seeding again. I did a merge back from develop and resolved a few minor conflicts. |
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 work! I am quite convinced by this adjoint-tangent approach. I made a few comments, porting mostly on minor details.
# Seeding | ||
nmap = length(H.map) | ||
t1sseedvec = zeros(Float64, length(x)) | ||
for i in 1:nmap |
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 maybe add an @inbounds
macro there
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. The bounds checking actually creates a real overhead. So I'm not sure whether to use it everywhere or not.
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.
In fact, even if bounds checking creates an overhead, it is actually negligible on modern architecture, as far as I observed
src/polar/kernels.jl
Outdated
F[npq + i] += coef_cos * sin_val - coef_sin * cos_val | ||
end | ||
if i > npv | ||
F[npq + i] += coef_cos * sin_val - coef_sin * cos_val |
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 understand the following code. It looks like to me that this line is redundant with the line 146 above?
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.
Oops. Fixed.
adj_v_a[to] -= adj_aij | ||
end | ||
# qinj is not active | ||
# if i > npv |
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.
Maybe remove dead comment? Or explain why we are keeping it from now?
dc6cd20
to
fe8d992
Compare
No description provided.