-
Notifications
You must be signed in to change notification settings - Fork 45
add oop jac forwarddiff_color_jacobian
#65
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
Conversation
huanglangwen
commented
Sep 22, 2019
Is it good for merge? @ChrisRackauckas |
|
||
function g(x) # out-of-place | ||
global fcalls += 1 | ||
dx = zero(x) |
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 should make this test be on static arrays instead, so it tests that no mutation is used by failing on mutation
for j in 1:chunksize | ||
col_index = (i-1)*chunksize + j | ||
(col_index > maxcolor) && return J | ||
J = J + partials.(vec(fx), j) .* [(i==col_index ? 1 : 0) for i in 1:ncols]' |
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.
there should be a way to remove the array allocation here.
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'm thinking of the delta tensor but that would introduce yet another dependency.
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.
The delta tensor? Is that a FillArrays.jl object?
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 thought there would be some lazy Kronecker delta tensors, but looks like there's no such thing. FillArrays.jl doesn't work, the slice of eye generates sparse vector and its product with a normal array would be sparseCSC which is worse.
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 can't find any to make a general way of removing this array allocation. If we focus at StaticArrays
, maybe I can make some overloads for +
.
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.
It would be fine to have a special branch for StaticArrays. How does ForwardDiff do it? It has a special routine for Jacobians that doesn't allocate.
To clarify the purpose here, the OOP version is really for two cases:
So we really just need to make sure that |