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

Switch from full Newton to modified Newton method in SUNDIALS #196

Merged
merged 4 commits into from
Jun 21, 2024

Conversation

sleweke-bayer
Copy link
Contributor

@sleweke-bayer sleweke-bayer commented May 17, 2024

Changes from full Newton to modified Newton in the time integrator. The Jacobian is only updated when SUNDIALS requests us to. This enables us to reuse the factorized Jacobian for multiple linear solves.

TODO

  • Add jacobian wrapper to update the Jacobian only when requested
  • Fix issues with IDAS failing (Increased the allowed error test fails to 10, which is the IDAS default)
  • Fix sensitivities: IDA_TOO_MUCH_WORK
  • Write jacobian() functions that only compute the Jacobian, not the residual and use that in jacobian wrapper?
  • Rigorous benchmarks: FV, DG, all transport models with linear and non-linear binding
  • Make full/modified Newton optional

@sleweke-bayer
Copy link
Contributor Author

sleweke-bayer commented May 22, 2024

Maybe we could figure out how to do that on the unit-operation level.
Then, finite volume discretizations can still do the full Newton.

@jbreue16
Copy link
Contributor

jbreue16 commented May 24, 2024

Update:

  • First benchmarks (linear and SMA for each 1D transport model, without sensitivities) show a big and consistent advantage for modified Newton for the FV code.
  • Some tests failing has nothing to do with forcing an update of the Jacobian, in fact thats already done with the IDASReInit call.
  • Our max error test fails default to 7, theres a mistake in the SUNDIALS v3.2 documentation where its reported as 7 (and 10 at another place), the actual default here is 10. Setting this to 10 fixes the tests that fail because of non-finished simulations
  • I have decided to also adjust the max newton iterations to 4, which is also the IDAS default, even though that didnt cause any troubles for the tests.
  • Sensitivities might work better with the full Newton method. There was no problem for linear models but for our standard LWE case. Here, IDAS terminates with IDAS_TOO_MUCH_WORK for all transport models. By setting MAX_STEPS higher and higher, the simulations finally terminate but with excessive compute times. I have no explanation for this rn, full Newton simply being that much more efficient here is counter intuitive for me. Any ideas on that?
  • As expected, I had to fix some numerical tolerances in some tests

As it is right now, we need to make the modified/full Newton optional, with default to the latter if sensitivities are involved.

@sleweke
Copy link
Contributor

sleweke commented May 26, 2024

  • Sensitivities might work better with the full Newton method.

I've noticed that the Jacobian was not evaluated when residualSens() was called.
This is now fixed, please test again.

sleweke-bayer and others added 4 commits June 21, 2024 14:00
Enables the (optional) use of modified Newton in the time integrator.
The Jacobian is only updated when SUNDIALS requests us to. This enables
us to reuse the factorized Jacobian for multiple linear solves.
Modified Newton method fails for old (more restrictive) set of
parameters. Only Full Newton method was able to always converge with the
old parameters. We thus switched to the IDAS defaults, which is
specifically defined for modified Newton method
Enable call of residualimplementation function that only computes the
Jacobian, but not the residual via templates

Recompute (static) DG Jacobian in every pure Jacobian function call

Small clean up of DG units
@jbreue16 jbreue16 merged commit 487a3fa into cadet:master Jun 21, 2024
3 checks passed
jbreue16 pushed a commit that referenced this pull request Jun 21, 2024
Enables the (optional) use of modified Newton in the time integrator.
The Jacobian is only updated when SUNDIALS requests us to. This enables
us to reuse the factorized Jacobian for multiple linear solves.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants