JuliaDiff / SparseDiffTools.jl

Fast jacobian computation through sparsity exploitation and matrix coloring
MIT License
237 stars 41 forks source link

have update_coeffs(L::ADVecProd,) recursively update L.f #232

Closed vpuri3 closed 1 year ago

vpuri3 commented 1 year ago

https://github.com/SciML/OrdinaryDiffEq.jl/pull/1917

vpuri3 commented 1 year ago

@gaurav-arya, can you take this over and write a few tests in test/test_jaches_products.jl that utilize this functionality?

ie

change

f(y, x) = mul!(y, A, x)
f(x) = A * x

to something something like UJacobianWrapper

struct Func
  u
  p
  t
end

function update_coefficients!(f::Fun, u, p, t)
# update u, p, t
end

function update_coefficients(f::Fun, u, p, t)
# update u, p, t
end

(f::Fun)(x) = t * p * u
codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 62.50% and project coverage change: +2.24 :tada:

Comparison is base (53755d8) 82.52% compared to head (57f4a55) 84.76%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #232 +/- ## ========================================== + Coverage 82.52% 84.76% +2.24% ========================================== Files 14 14 Lines 944 952 +8 ========================================== + Hits 779 807 +28 + Misses 165 145 -20 ``` | [Impacted Files](https://codecov.io/gh/JuliaDiff/SparseDiffTools.jl/pull/232?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff) | Coverage Δ | | |---|---|---| | [src/differentiation/vecjac\_products.jl](https://codecov.io/gh/JuliaDiff/SparseDiffTools.jl/pull/232?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff#diff-c3JjL2RpZmZlcmVudGlhdGlvbi92ZWNqYWNfcHJvZHVjdHMuamw=) | `88.88% <55.55%> (+29.27%)` | :arrow_up: | | [src/differentiation/jaches\_products.jl](https://codecov.io/gh/JuliaDiff/SparseDiffTools.jl/pull/232?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff#diff-c3JjL2RpZmZlcmVudGlhdGlvbi9qYWNoZXNfcHJvZHVjdHMuamw=) | `95.37% <66.66%> (-0.46%)` | :arrow_down: | | [ext/SparseDiffToolsZygote.jl](https://codecov.io/gh/JuliaDiff/SparseDiffTools.jl/pull/232?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff#diff-ZXh0L1NwYXJzZURpZmZUb29sc1p5Z290ZS5qbA==) | `100.00% <100.00%> (+17.14%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

vpuri3 commented 1 year ago

thanks. can you also add these tests to vecjac?

gaurav-arya commented 1 year ago

@vpuri3 is the CI working properly? It doesn't seem to have caught the bugs due to ArrayInterface v7, and moreover there's a ref to an object here that doesn't exist:

https://github.com/JuliaDiff/SparseDiffTools.jl/blob/f191085e7ab4c5281fb80e4c779ae440b5dcc3cc/test/test_vecjac_products.jl#L27

vpuri3 commented 1 year ago

remove zygotejacvec

vpuri3 commented 1 year ago

file an issue to fix ci after this pr please

gaurav-arya commented 1 year ago

234

gaurav-arya commented 1 year ago

@ChrisRackauckas this PR's also ready for review

gaurav-arya commented 1 year ago

@ChrisRackauckas this is ready for another review. your comment is resolved (see above). There are a few cleanup points for a separate PR (https://github.com/JuliaDiff/SparseDiffTools.jl/issues/238), but the behaviour here should now be correct, and ready for ODE.jl.

Also: while making the tests more comprehensive I found that x and v ended up becoming identical to each other a lot in test_jaches_products.jl which resulted in the incorrect behaviour you caught not being picked up in tests. I fixed that, and also fixed an issue that num_hesvec! and friends did not restore x to its original state when mutating them, and then tightened up some unnecessarily loose tolerances that might have been in place because of this. All of this in done in c126bf8cb0, apologies for the additional diff noise: I needed these changes here in order to have comprehensive testing.

vpuri3 commented 1 year ago

@ChrisRackauckas ping