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

Feature/default order 4 ark #345

Closed
wants to merge 17 commits into from

Conversation

Steven-Roberts
Copy link
Collaborator

Changes the default ERK and DIRK integrators to improve robustness of method and embedding

johnwparent and others added 11 commits September 12, 2023 20:23
example_utilities.hpp requires `stoi` and similar methods. These are
declared in `<string>`, but `<string>` is not included. This means this
file will break any TU it's included in.

Resolves #330

Co-authored-by: Cody Balos <balos1@llnl.gov>
Fixes #335

Co-authored-by: David Gardner <gardner48@llnl.gov>
Fixes #333. Some other typedefs already had specialized (towards idas or
cvodes) prefixed names, it won't harm to generalize this.

---------

Co-authored-by: Cody Balos <balos1@llnl.gov>
Add scripts for comparing benchmark and example timings against release timings and a notebook using Thicket to visualize data

---------

Co-authored-by: David J. Gardner <gardner48@llnl.gov>
Co-authored-by: Cody J. Balos <balos1@llnl.gov>
Fixes part of #253 by avoiding nested loops that take `O(M*N)` operations.

Also fixes #256.

---------

Signed-off-by: phannebohm <phannebohm@fh-bielefeld.de>
Co-authored-by: Cody Balos <balos1@llnl.gov>
Copy link
Collaborator

@drreynolds drreynolds left a comment

Choose a reason for hiding this comment

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

The improvements from switching out these default integrators ranges from dramatic to marginal (most problems improve dramatically).

My one question is: how much testing with various RK table options did you do before settling on these specific default tables?

Also, you'll need to edit the CHANGELOG.md file, as well as the "recent changes" section in ARKODE's Introduction.rst file. In those, I recommend adding a sentence to tell users how to switch back to the previous default.

@Steven-Roberts
Copy link
Collaborator Author

Steven-Roberts commented Sep 25, 2023

how much testing with various RK table options did you do before settling on these specific default tables?

I specifically chose these method due to there optimized properties and so that the number of explicit stages or implicit solves would be the same as the old defaults. Thus, the per step cost will likely not change significantly, but the accuracy or embedding quality will be better. This constraint doesn't leave many options, and I only tested the methods in this PR with the old defaults. If you have candidate methods you think might be better defaults, I can run some comparisons.

@drreynolds
Copy link
Collaborator

how much testing with various RK table options did you do before settling on these specific default tables?

I specifically chose these method due to there optimized properties and so that the number of explicit stages or implicit solves would be the same as the old defaults. Thus, the per step cost will likely not change significantly, but the accuracy or embedding quality will be better. This constraint doesn't leave many options, and I only tested the methods in this PR with the old defaults. If you have candidate methods you think might be better defaults, I can run some comparisons.

This is fine (at least for now). I'm working on updates to my regression-testing repository for ARKODE, that would hopefully allow us to do more thorough testing of different methods. That said, given the improvement here, I think that we can approve this PR, and if needed we can update the default method again if we find something better before the next release.

@balos1
Copy link
Member

balos1 commented Sep 26, 2023

This PR will need a corresponding update of https://github.com/sundials-codes/answers to update the output files used in the GitHub actions CI.

Steven-Roberts and others added 5 commits September 26, 2023 18:11
Fix missing soversion on some SUNLinSol and SUNNonlinSol targets

---------

Signed-off-by: Julien Schueller <schueller@phimeca.com>
Co-authored-by: Cody Balos <balos1@llnl.gov>
1. Adds missing implicit tables to the ARKODE constants docs
2. Makes formatting of constants consistent in ARKODE docs
3. Groups implicit tables by order

---------

Co-authored-by: David Gardner <gardner48@llnl.gov>
Update ARKStepSetTableNum to recognize ARK2-3-1-2 as a valid ARK pair.
Copy link
Collaborator

@drreynolds drreynolds left a comment

Choose a reason for hiding this comment

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

I have two requests on this PR, but once those are addressed then I think that it can safely be merged.

include/arkode/arkode_butcher_dirk.h Outdated Show resolved Hide resolved
include/arkode/arkode_butcher_erk.h Outdated Show resolved Hide resolved
drreynolds
drreynolds previously approved these changes Oct 20, 2023
@balos1 balos1 removed this from the SUNDIALS Next milestone Oct 24, 2023
balos1 pushed a commit that referenced this pull request Oct 26, 2023
Only adding the `Sofroniou-Spaletta-5-3-4` method but not changing the
defaults like #345

---------

Co-authored-by: Daniel R. Reynolds <reynolds@smu.edu>
Co-authored-by: David Gardner <gardner48@llnl.gov>
@balos1 balos1 removed the dont-merge label Nov 28, 2023
gardner48 added a commit that referenced this pull request Dec 18, 2023
Only adding the `Sofroniou-Spaletta-5-3-4` method but not changing the
defaults like #345

---------

Co-authored-by: Daniel R. Reynolds <reynolds@smu.edu>
Co-authored-by: David Gardner <gardner48@llnl.gov>
balos1 pushed a commit that referenced this pull request Dec 18, 2023
Only adding the `Sofroniou-Spaletta-5-3-4` method but not changing the
defaults like #345

---------

Co-authored-by: Daniel R. Reynolds <reynolds@smu.edu>
Co-authored-by: David Gardner <gardner48@llnl.gov>
@gardner48 gardner48 dismissed drreynolds’s stale review December 18, 2023 22:48

The merge-base changed after approval.

@gardner48 gardner48 marked this pull request as draft June 21, 2024 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants