Skip to content
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

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

Merged
merged 17 commits into from
Apr 8, 2023

Conversation

vpuri3
Copy link
Contributor

@vpuri3 vpuri3 commented Mar 27, 2023

@vpuri3
Copy link
Contributor Author

vpuri3 commented Mar 27, 2023

@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
Copy link

codecov bot commented Mar 27, 2023

Codecov Report

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

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

Additional details and impacted files
@@            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 Coverage Δ
src/differentiation/vecjac_products.jl 88.88% <55.55%> (+29.27%) ⬆️
src/differentiation/jaches_products.jl 95.37% <66.66%> (-0.46%) ⬇️
ext/SparseDiffToolsZygote.jl 100.00% <100.00%> (+17.14%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@vpuri3
Copy link
Contributor Author

vpuri3 commented Mar 27, 2023

thanks. can you also add these tests to vecjac?

@gaurav-arya
Copy link
Contributor

@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:

L = ZygoteVecJac(f, x)

@vpuri3
Copy link
Contributor Author

vpuri3 commented Mar 27, 2023

remove zygotejacvec

@vpuri3
Copy link
Contributor Author

vpuri3 commented Mar 27, 2023

file an issue to fix ci after this pr please

@gaurav-arya
Copy link
Contributor

#234

src/differentiation/vecjac_products.jl Show resolved Hide resolved
test/test_jaches_products.jl Show resolved Hide resolved
test/test_jaches_products.jl Outdated Show resolved Hide resolved
end

(w::WrapFunc)(u) = sum(w.u) * w.p * w.t * w.func(u)
function (w::WrapFunc)(v, u)
Copy link
Contributor

Choose a reason for hiding this comment

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

@vpuri3 can you take a look at how this operator is using u as part of its state, and applying it to a different RHS v? Is this something that is invalid in your opinion? Because your recent changes don't work with this -- this also relates to the discussion in SciML/SciMLOperators.jl#57.

Copy link
Contributor

Choose a reason for hiding this comment

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

(This is a separate, SciMLOperators issue, so resolved for our purposes here)

@gaurav-arya gaurav-arya force-pushed the update_coeffs branch 4 times, most recently from 6d73989 to cc60333 Compare March 28, 2023 04:03
@gaurav-arya gaurav-arya mentioned this pull request Mar 28, 2023
3 tasks
@gaurav-arya gaurav-arya force-pushed the update_coeffs branch 3 times, most recently from b6aa232 to b072b13 Compare March 28, 2023 18:01
@gaurav-arya
Copy link
Contributor

@ChrisRackauckas this PR's also ready for review

@gaurav-arya gaurav-arya mentioned this pull request Mar 28, 2023
@gaurav-arya
Copy link
Contributor

gaurav-arya commented Mar 29, 2023

@ChrisRackauckas this is ready for another review. your comment is resolved (see above). There are a few cleanup points for a separate PR (#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 c126bf8, apologies for the additional diff noise: I needed these changes here in order to have comprehensive testing.

@@ -110,6 +111,7 @@ function numauto_hesvec!(dy,
g(cache1, x)
@. x -= 2ϵ * v
g(cache2, x)
@. x += ϵ * v
Copy link
Contributor

@gaurav-arya gaurav-arya Mar 29, 2023

Choose a reason for hiding this comment

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

@ChrisRackauckas that this was not done before seems to actually have been a fairly serious numerical error: the vector x where the Jacobian is being evaluated at gets edited by around sqrt(eps) each time the Jacobian is applied, if I am thinking this through correctly

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, not Jacobian; this error was only present for (finite difference computed) Hessians

@vpuri3
Copy link
Contributor Author

vpuri3 commented Apr 7, 2023

@ChrisRackauckas ping

@ChrisRackauckas ChrisRackauckas merged commit c337dd5 into JuliaDiff:master Apr 8, 2023
@vpuri3 vpuri3 deleted the update_coeffs branch April 11, 2023 16:34
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.

None yet

3 participants