Update SUNDIALS support #2885
Update SUNDIALS support #2885
Conversation
master branch will be 5.1.1 not 5.2.0
- Don't pin pip and setuptools versions - Bump netcdf4 and Cython - Install boututils and boutdata
CI: Replace pip script with requirements.txt
Set oversubscribe flags for openmpi 5
Bumps [actions/setup-python](https://github.com/actions/setup-python) from 4 to 5. - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@v4...v5) --- updated-dependencies: - dependency-name: actions/setup-python dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [externalpackages/boutdata](https://github.com/boutproject/boutdata) from `908a4c2` to `a043a2b`. - [Release notes](https://github.com/boutproject/boutdata/releases) - [Commits](boutproject/boutdata@908a4c2...a043a2b) --- updated-dependencies: - dependency-name: externalpackages/boutdata dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [externalpackages/googletest](https://github.com/google/googletest) from `829c199` to `dddb219`. - [Release notes](https://github.com/google/googletest/releases) - [Commits](google/googletest@829c199...dddb219) --- updated-dependencies: - dependency-name: externalpackages/googletest dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
…b_actions/actions/setup-python-5 Bump actions/setup-python from 4 to 5
…dules/externalpackages/googletest-dddb219 Bump externalpackages/googletest from `829c199` to `dddb219`
…dules/externalpackages/boutdata-a043a2b Bump externalpackages/boutdata from `908a4c2` to `a043a2b`
…aster Remove propietary data (master)
Do not excessively bump googletest
Bumps [actions/cache](https://github.com/actions/cache) from 3 to 4. - [Release notes](https://github.com/actions/cache/releases) - [Changelog](https://github.com/actions/cache/blob/main/RELEASES.md) - [Commits](actions/cache@v3...v4) --- updated-dependencies: - dependency-name: actions/cache dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [ZedThree/clang-tidy-review](https://github.com/zedthree/clang-tidy-review) from 0.14.0 to 0.17.0. - [Release notes](https://github.com/zedthree/clang-tidy-review/releases) - [Changelog](https://github.com/ZedThree/clang-tidy-review/blob/master/CHANGELOG.md) - [Commits](ZedThree/clang-tidy-review@v0.14.0...v0.17.0) --- updated-dependencies: - dependency-name: ZedThree/clang-tidy-review dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
…b_actions/actions/cache-4 Bump actions/cache from 3 to 4
…b_actions/ZedThree/clang-tidy-review-0.17.0 Bump ZedThree/clang-tidy-review from 0.14.0 to 0.17.0
Add warnings about SUNDIALS versions <4
Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 3 to 4. - [Release notes](https://github.com/codecov/codecov-action/releases) - [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md) - [Commits](codecov/codecov-action@v3...v4) --- updated-dependencies: - dependency-name: codecov/codecov-action dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [externalpackages/googletest](https://github.com/google/googletest) from `dddb219` to `b75ecf1`. - [Release notes](https://github.com/google/googletest/releases) - [Commits](google/googletest@dddb219...b75ecf1) --- updated-dependencies: - dependency-name: externalpackages/googletest dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
…b_actions/codecov/codecov-action-4 Bump codecov/codecov-action from 3 to 4
…dules/externalpackages/googletest-b75ecf1 Bump externalpackages/googletest from `dddb219` to `b75ecf1`
Bumps [externalpackages/boutdata](https://github.com/boutproject/boutdata) from `a043a2b` to `9e603a2`. - [Release notes](https://github.com/boutproject/boutdata/releases) - [Commits](boutproject/boutdata@a043a2b...9e603a2) --- updated-dependencies: - dependency-name: externalpackages/boutdata dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
…dules/externalpackages/boutdata-9e603a2 Bump externalpackages/boutdata from `a043a2b` to `9e603a2`
Bumps [externalpackages/googletest](https://github.com/google/googletest) from `b75ecf1` to `e4fdb87`. - [Release notes](https://github.com/google/googletest/releases) - [Commits](google/googletest@b75ecf1...e4fdb87) --- updated-dependencies: - dependency-name: externalpackages/googletest dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
…dules/externalpackages/boutdata-cd0bc40 Bump externalpackages/boutdata from `9e603a2` to `cd0bc40`
…dules/externalpackages/googletest-b479e7a Bump externalpackages/googletest from `e4fdb87` to `b479e7a`
|
Failing tests are all unrelated. Does this replaces #2863 ? |
Yes |
|
I'm putting this in draft status while I test a few final things and so @drreynolds (also from the SUNDIALS team) can review. |
| switch (adap_method) { | ||
| case 0: | ||
| controller = SUNAdaptController_PID(suncontext); | ||
| break; | ||
| case 1: | ||
| controller = SUNAdaptController_PI(suncontext); | ||
| break; | ||
| case 2: | ||
| controller = SUNAdaptController_I(suncontext); | ||
| break; | ||
| case 3: | ||
| controller = SUNAdaptController_ExpGus(suncontext); | ||
| break; | ||
| case 4: | ||
| controller = SUNAdaptController_ImpGus(suncontext); | ||
| break; | ||
| case 5: | ||
| controller = SUNAdaptController_ImExGus(suncontext); | ||
| break; |
There was a problem hiding this comment.
Do you really want to hard-code all of these into the BOUT++ interface? Given that 5 of these are just the Soderlind controller with different constants, would you prefer to just have two adapt_method options, along with specification of the controller parameters?
Or perhaps since BOUT++ already had support for this set of adapt_method values, you retained its current functionality? It might be worthwhile to ask the BOUT++ folks if they ever used this, and if not, to streamline the interface?
There was a problem hiding this comment.
I only did this to preserve existing capabilities while preventing compiler warning with SUNDIALS 6.7+. You're suggestion sounds cleaner, but users will probably want to pick a controller by name or index over looking up what controller parameters to use. I'll think about our options and see if the BOUT++ developers have a preference. @bendudson?
Co-authored-by: Daniel R. Reynolds <reynolds@smu.edu>
There was a problem hiding this comment.
Thanks @Steven-Roberts! LGTM, a couple of comments to look at.
| MAYBE_UNUSED(SUNContext sunctx)) { | ||
| return SUNNonlinSol_Newton(y); | ||
| template<typename Func, typename... Args> | ||
| inline decltype(auto) callWithSUNContext(Func f, |
There was a problem hiding this comment.
Nice! I wonder if it's worth trying to also bring the error checking into this wrapper too? Most (but not all, unfortunately) of the fs we call return nullptr on error and then we throw BoutException -- does it clarify/simplify things at the call site if we move that logic here? Probably not, but might be worth having a look?
There was a problem hiding this comment.
We could, but we would either have a generic error message or have to pass in a string for a useful error message. Since SUNDIALS v6, we can attach an error handler to the SUNContext which would be called on any SUNDIALS error. Unfortunately, it's not available in SUNDIALS v4-5. Personally, I think we should eliminate the C-style error check after each SUNDIALS call and use the error handler. Then the cost of using old SUNDIALS versions would be worse error reporting. If that's a dealbreaker, I totally understand.
Co-authored-by: Peter Hill <zed.three@gmail.com>
|
Replaced by #2896 |
Adds support for SUNDIALS v7 and drop suppose for versions <4