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

Fix SUNDIALS version guard and some deprecated function calls #2863

Closed
wants to merge 6 commits into from

Conversation

ZedThree
Copy link
Member

Fixes an issue with one version guard (caught versions < x.2 instead of < 3.2)

Also fixes some deprecated function calls. Turns out SUNDIALS v4 introduced a bunch of functions we should've been calling, although they were only recently actually deprecated.

There is one remaining call to a deprecated function (deprecated in 5.7):

src/solver/impls/arkode/arkode.cxx:351:33: warning: ‘int ARKStepSetAdaptivityMethod(void*, int, int, int, realtype*)’ is deprecated: use SUNAdaptController instead [-Wdeprecated-declarations]
  351 |   if (ARKStepSetAdaptivityMethod(arkode_mem, adap_method, 1, 1, nullptr) != ARK_SUCCESS) {
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Unfortunately the fix is not exactly straightforward, and it's not really from the docs how to upgrade.

bendudson
bendudson previously approved these changes Feb 12, 2024
Copy link
Contributor

@bendudson bendudson left a comment

Choose a reason for hiding this comment

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

Thanks @ZedThree ! @Steven-Roberts is working on the upgrade, so we'll hopefully support SUNDIALS v7, and drop support for really old versions,

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/solver/impls/ida/ida.cxx Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@Steven-Roberts
Copy link
Contributor

Good catch with the version guard. Yes, I'm working on a branch which will drop SUNDIALS <v4 support, add v7 support, and fix all the deprecated warnings including ARKStepSetAdaptivityMethod. I think that would supersede this, but here's the current diff (still a work in progress)

@ZedThree
Copy link
Member Author

Looking at what versions of SUNDIALS are available across OSes, it seems most have >= 5.0, so we could use that as our minimum version?

There's still a couple of OSes (notably Ubuntu 20 LTS) that only have SUNDIALS 3, but we do have the option to download and build SUNDIALS as part of BOUT++, so this is probably not a massive problem. There's also a couple of OSes that have separate packages for SUNDIALS 2, but I doubt this is worth continuing to support.

@Steven-Roberts
Copy link
Contributor

If we support SUNDIALS 5.0, I don't think there are any extra shims or special cases we need to handle to also support 4.0. Considering that BOUT++ is currently incompatible with SUNDIALS <4.0 and presumably this has not caused issues with users, it seems natural to me to pick 4.0 as the new minimum version to support. Just let me know @bendudson and @ZedThree what you want to go with.

@bendudson
Copy link
Contributor

Thanks @Steven-Roberts ! It sounds like >= 4.0 is a good choice for now. I wouldn't spend a lot of time on supporting older versions though: We can recommend users configure BOUT++ to download and compile a compatible SUNDIALS version.

@ZedThree
Copy link
Member Author

Fixed by #2896

@ZedThree ZedThree closed this Apr 11, 2024
@ZedThree ZedThree deleted the fix-sundials-guards-deprecations branch April 11, 2024 13:57
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.

3 participants