Fix inability to use sparse Jacobian with colorvec for OOP #1049

mauro3 · 2020-02-28T10:14:24Z

As discussed on slack, this allows to use a sparse jacobian for out-of-place objective functions. This works for my case but probably could be done more general.

The solve time of https://gist.github.com/mauro3/e85c85a4257a46ed73999b08f9b80ec2 (a FD PDE code) improves from >100s to <1s with this PR.

coveralls · 2020-02-28T11:17:45Z

Coverage decreased (-2.1%) to 84.793% when pulling c68e2ba on mauro3:m3/sparse_jac into d565aed on JuliaDiffEq:master.

codecov · 2020-02-28T11:18:15Z

Codecov Report

Merging #1049 into master will increase coverage by 0.14%.
The diff coverage is 94.44%.

@@            Coverage Diff             @@
##           master    #1049      +/-   ##
==========================================
+ Coverage   76.87%   77.02%   +0.14%     
==========================================
  Files          95       95              
  Lines       31107    31044      -63     
==========================================
- Hits        23914    23911       -3     
+ Misses       7193     7133      -60

Impacted Files	Coverage Δ
src/derivative_wrappers.jl	`88.23% <100%> (+5.19%)`	⬆️
src/derivative_utils.jl	`74.82% <85.71%> (+0.35%)`	⬆️
src/caches/kencarp_kvaerno_caches.jl	`92.3% <0%> (-1.1%)`	⬇️
src/integrators/integrator_utils.jl	`88.05% <0%> (-1%)`	⬇️
src/alg_utils.jl	`27.09% <0%> (-0.23%)`	⬇️
src/solve.jl	`92.5% <0%> (+20.51%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d565aed...c68e2ba. Read the comment docs.

ChrisRackauckas · 2020-02-28T12:31:33Z

Thanks! Can we get a test on this?

mauro3 · 2020-02-28T13:34:24Z

Which file to add the test?

ChrisRackauckas · 2020-02-28T16:28:55Z

https://github.com/JuliaDiffEq/OrdinaryDiffEq.jl/blob/9ce1fe0a673ada3167ea6f4762964c28b3e89ea7/test/interface/sparsediff_tests.jl

YingboMa

Thanks a lot! Maybe we should add the interface factorization(A) -> FactorizationType in ArrayInterface.

ChrisRackauckas · 2020-02-28T16:50:06Z

Thanks a lot! Maybe we should add the interface factorization(A) -> FactorizationType in ArrayInterface.

Yeah, that's what would need to happen.

mauro3 · 2020-02-28T19:59:47Z

I'll add some tests and update the PR to use Yingbo's ArrayInterface changes.

mauro3 · 2020-02-28T20:36:01Z

test/interface/sparsediff_tests.jl

+        for prob in map(f->ODEProblem(f,u0,tspan),
+                        [ODEFunction(f,colorvec=colors,jac_prototype=jac_sp),
+                         ODEFunction(f,jac_prototype=jac_sp),
+                         #ODEFunction(f,colorvec=colors) # this one fails both the u[end] and length tests


Any idea why the commented-out test would behave differently?

Because one must have sparsity to get sparse jacobian work appropriately.

Is this still the case now with our update? Because some tests still behave erratically.

mauro3 · 2020-02-28T20:37:39Z

This will now fail tests because it needs JuliaArrays/ArrayInterface.jl#39.

I added some tests but I don't know to test the type of the used Jacobian.

Oh, and I should add OOP tests...

Edit: ok some tests are failing locally, so, it will need some more work.

- tests are failing for SDIRK methods, needs investigating

test/interface/sparsediff_tests.jl

mauro3 · 2020-03-02T21:44:08Z

test/interface/sparsediff_tests.jl

+          isbroken = i==3 && (
+            (f, ad, tol) == (f_oop, true, nothing) ||
+            (f, ad, Solver, tol) == (f_oop, false, Trapezoid, nothing) ||
+            (f, Solver, tol) == (f_ip, Trapezoid, nothing) ||
+            (f, Solver) == (f_oop, Rodas5) ||
+            (f, Solver) == (f_ip, Rodas5) ||
+            (f, Solver) == (f_oop, KenCarp4) ||
+            (f, Solver) == (f_ip, KenCarp4)
+          )


These failing tests need investigation.

I wouldn't expect colors without sparsity to work.

IMO, the color stuff should be independent of the matrix type of J.

These are still "broken".

mauro3 · 2020-03-02T21:46:20Z

The tests failed because the Jacobians sometimes got converted to dense arrays. I updated all the instances where I think that occurred. However, I do not fully understand the code, so this needs a careful review!

Also note that with JuliaArrays/ArrayInterface.jl#39, CI should now pass. (Although I haven't run the full test suite locally... but the new tests did pass locally.)

ChrisRackauckas · 2020-03-02T23:42:01Z

test/interface/sparsediff_tests.jl

+                         ODEFunction(f,colorvec=colors)
+                         ]))
+          isbroken = i==3 && (
+            (f, ad, tol) == (f_oop, true, nothing) ||


all with AD fail?

Only those with ODEFunction(f,colorvec=colors), and Trapezoid passes there too. Note that they are not completely wrong, just about a factor 10 larger (different) error.

I had a closer look at the broken tests:maximum(sol_std.u[end].-sol.u[end]) is around 1e-5 for tol=1e-10 and around 1e-3 for tol=nothing.

mauro3 · 2020-03-03T10:11:39Z

It looks like that this touches a lot of the code from PR #937 of @huanglangwen. Maybe @huanglangwen has time to look at this?

src/derivative_wrappers.jl

mauro3 · 2020-03-04T21:43:19Z

Tests will fail because it now needs SciML/DiffEqBase.jl#449.

src/derivative_utils.jl

mauro3 · 2020-03-05T12:10:20Z

The tests pass (apart from https://travis-ci.org/JuliaDiffEq/OrdinaryDiffEq.jl/jobs/658621217?utm_medium=notification&utm_source=github_status, which is due to SciML/DiffEqBase.jl#449 needing changes in DelayDiffEq) and I think this is ready to be merged.

For the broken tests, I would suggest to create an issue to not forget about them.

Fix inability to use sparse Jacobian with colorvec for OOP

343281d

YingboMa approved these changes Feb 28, 2020

View reviewed changes

YingboMa mentioned this pull request Feb 28, 2020

Add lu_instance JuliaArrays/ArrayInterface.jl#39

Merged

mauro3 commented Feb 28, 2020

View reviewed changes

mauro3 force-pushed the m3/sparse_jac branch 2 times, most recently from e30ce3e to 8ac8574 Compare February 28, 2020 22:04

Using ArrayInterface.lu_instance and added tests

b7b1cb6

- tests are failing for SDIRK methods, needs investigating

mauro3 force-pushed the m3/sparse_jac branch from 8ac8574 to b7b1cb6 Compare February 28, 2020 22:13

mauro3 commented Feb 28, 2020

View reviewed changes

test/interface/sparsediff_tests.jl Outdated Show resolved Hide resolved

mauro3 commented Feb 28, 2020

View reviewed changes

test/interface/sparsediff_tests.jl Outdated Show resolved Hide resolved

Jacobian calculation and caches needed updates too.

3e55823

mauro3 commented Mar 2, 2020

View reviewed changes

mauro3 requested a review from YingboMa March 2, 2020 21:46

ChrisRackauckas reviewed Mar 2, 2020

View reviewed changes

huanglangwen reviewed Mar 3, 2020

View reviewed changes

src/derivative_wrappers.jl Show resolved Hide resolved

Add sparsity as option to ODEFunction

f60dfd8

mauro3 mentioned this pull request Mar 4, 2020

Fixes chunksize #1054

Merged

ChrisRackauckas closed this Mar 4, 2020

ChrisRackauckas reopened this Mar 4, 2020

YingboMa reviewed Mar 5, 2020

View reviewed changes

src/derivative_utils.jl Outdated Show resolved Hide resolved

YingboMa reviewed Mar 5, 2020

View reviewed changes

src/derivative_utils.jl Outdated Show resolved Hide resolved

Yingbo's comments and version bound on DiffEqBase

c68e2ba

ChrisRackauckas merged commit 0863dab into SciML:master Mar 5, 2020

Uh oh!

Fix inability to use sparse Jacobian with colorvec for OOP #1049

Fix inability to use sparse Jacobian with colorvec for OOP #1049

Uh oh!

Conversation

mauro3 commented Feb 28, 2020

Uh oh!

coveralls commented Feb 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ChrisRackauckas commented Feb 28, 2020

Uh oh!

mauro3 commented Feb 28, 2020

Uh oh!

ChrisRackauckas commented Feb 28, 2020

Uh oh!

YingboMa left a comment

Choose a reason for hiding this comment

Uh oh!

ChrisRackauckas commented Feb 28, 2020

Uh oh!

mauro3 commented Feb 28, 2020

Uh oh!

mauro3 Feb 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mauro3 commented Feb 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mauro3 commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mauro3 commented Mar 3, 2020

Uh oh!

Uh oh!

mauro3 commented Mar 4, 2020

Uh oh!

Uh oh!

Uh oh!

mauro3 commented Mar 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Feb 28, 2020 •

edited

Loading

codecov bot commented Feb 28, 2020 •

edited

Loading

mauro3 Feb 28, 2020 •

edited

Loading

mauro3 commented Feb 28, 2020 •

edited

Loading

mauro3 commented Mar 2, 2020 •

edited

Loading