Skip to content

Conversation

@kanav99
Copy link
Contributor

@kanav99 kanav99 commented Jul 10, 2020

Let's see how test turns out, and move to sparse ones next

src/jacobians.jl Outdated
dx = (vecfx1-vecfx)/epsilon
J = J + _make_Ji(J, eltype(x), dx, color_i, nrows, ncols)
if jac_prototype isa Nothing
J = hcat(J, dx)
Copy link
Member

Choose a reason for hiding this comment

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

doing a mapreduce would likely perform the hcat in a smarter way IIRC. This won't allocate as much as before, but it will still allocate quite a bit.

@kanav99
Copy link
Contributor Author

kanav99 commented Jul 10, 2020

I think central differencing is broken?

f(u) = [u[1], u[2], u[1]]
FiniteDiff.finite_difference_jacobian(f, [1.0,2.0], Val(:central))

@kanav99
Copy link
Contributor Author

kanav99 commented Jul 11, 2020

No it wasn't broken, I used wrong argument. How does this look like now @ChrisRackauckas ?

@ChrisRackauckas
Copy link
Member

Can you test this locally with OrdinaryDiffEq.jl before we tag and merge? Last time it seems like we hit something that was missing from the FiniteDiff tests

@kanav99
Copy link
Contributor Author

kanav99 commented Jul 11, 2020

Yes, I have started one

end

if jac_prototype isa Nothing
if jac_prototype isa Nothing && sparsity isa Nothing
Copy link
Member

Choose a reason for hiding this comment

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

is there a specific file in OrdinaryDiffEq.jl that we should include as a downstream test here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

yes that's a good one. Can you setup an environment which does a downstream test to that? That should hopefully catch any outrageous issues.

@ChrisRackauckas ChrisRackauckas merged commit b9110cb into JuliaDiff:master Jul 11, 2020
@ChrisRackauckas
Copy link
Member

I will merge but let's not tag until we have a bit of downstream testing.

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.

2 participants