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

Support NonlinearSolve.jl for the ODE nonlinear solver #2167

Merged

Conversation

oscardssmith
Copy link
Contributor

This is WIP, but it mostly works (for in place ODE only), (and is a lot slower since it's missing all the fancy optimizations still).

@oscardssmith
Copy link
Contributor Author

so the current status is that out of place solves now work (although something is going wrong since it is getting a lot of nonlinear solver convergence failures). In place is broken because _compute_rhs! isn't autodiff compatible, and no matter what I do, NonlinearSolve is trying to autodiff through it.

@avik-pal
Copy link
Member

avik-pal commented Apr 5, 2024

In place is broken because _compute_rhs! isn't autodiff compatible

How does the default newton in ordinarydiffeq handle this?

and is a lot slower since it's missing all the fancy optimizations still

you probably need the recompute_jacobian option here to mimic the jacobian reuse behavior https://github.com/SciML/NonlinearSolve.jl/blob/0dfc1358be16478a29e8940b5abca896d30ec9ff/src/core/generalized_first_order.jl#L204-L205

@oscardssmith
Copy link
Contributor Author

oscardssmith commented Apr 8, 2024

with recompute_jacobian=false this actually is looking really good! I'm getting better stats than NLNewton (admitadly so far I've only been testing on f(u,p,t)=u. That said, I do seem to have somehow messed up NLNewton` in the process...

@oscardssmith
Copy link
Contributor Author

it is worth mentioning that it is very possible that the better stats come from me not updating the stats properly. Still needs testing.

@ChrisRackauckas
Copy link
Member

It's missing tests?

@oscardssmith
Copy link
Contributor Author

oscardssmith commented Apr 23, 2024

I've added some tests that use this. I'm not sure how much needs testing.

@ChrisRackauckas
Copy link
Member

Match a bit of what NLAnderson has done https://github.com/search?q=repo%3ASciML%2FOrdinaryDiffEq.jl+NLAnderson&type=code

@avik-pal
Copy link
Member

Are we using the jacobian reuse here? or is that a plan for later

@oscardssmith
Copy link
Contributor Author

I believe everything should now work. The last issues were that tstep and invγdt were being computed correctly, but the incorrect values for them were being passed into the nlp_params, leading to all sorts of nonsense.

@ChrisRackauckas ChrisRackauckas merged commit 438f34b into SciML:master May 8, 2024
16 of 30 checks passed
@oscardssmith oscardssmith deleted the os/NonlinearSolve-nlsolve branch May 8, 2024 19:33
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