Simple fix for Sundials 5.1 compatibility #814

band-a-prend · 2020-02-28T22:26:45Z

Checklist

There is a clear use-case for this code change
The commit message has a short title & references relevant issues
Build passes (scons build & scons test) and unit tests address code coverage
The pull request is ready for review

If applicable, fill in the issue number this pull request is fixing

Fixes #

Changes proposed in this pull request

The Sundials 5.1 has no any changes in CVODES API in comparison with sundials 4.0, 4.1, 5.0 therefore the compatibility configuration fix is trivial.

*****************************
***    Testing Summary    ***
*****************************

Tests passed: 1046
Up-to-date tests skipped: 0
Tests failed: 0

*****************************

The Sundials 5.1 has no any changes in CVODES API in comparison with sundials 4.0, 4.1, 5.0 therefore the compatibility configuration fix is trivial.

band-a-prend · 2020-02-29T12:24:32Z

P.s.

Tested for system installed sundials.

I tried to make changes like in 94a7003
,but the submodule fetch sundials major release 5.0 instead of 5.1.

bryanwweber · 2020-03-03T21:47:53Z

@band-a-prend Thanks for submitting this! You can update the submodule by

cd ext/sundials
git fetch
git checkout **tag for 5.1.0**
cd ../..
git add .
git commit -m 'Update Sundials submodule'

After that, assuming no code in SUNDIALS has moved around, the other changes in 94a7003 should be straightforward.

band-a-prend · 2020-03-04T06:33:47Z

Thank you for the clue. The command git fetch in submodule directory didn't result in any thing for me and checkout didn't work after it. I have to use git submodule update --remote -- . to clone submodule repo and then git checkout v5.1.0. After that and going to root directory of cantera project the commited diff noticed a change in submodule sha1.

I push changes now and hope to check build process on my local machine this evening.

bryanwweber · 2020-03-04T17:13:16Z

Thanks @band-a-prend perhaps the remote was not configured for the submodule or something. Anyways, the change seems to be appropriate

band-a-prend · 2020-03-04T17:38:20Z

I recheck currently build and tests, please wait.

band-a-prend · 2020-03-04T18:38:33Z

Appropriate sha1-snapshot (b78e575639babe99aea9a0558d0f64732d6d729e) of sundials tag v5.1.0 is fetched now into ext/sundials by SConstruct script. The build with options python_package = 'full' f90_interface = 'y' system_eigen = 'y' system_sundials = 'n' extra_inc_dirs = '/usr/include/eigen3' is successfully done. Tests are passed:

*****************************
***    Testing Summary    ***
*****************************

Tests passed: 1046
Up-to-date tests skipped: 0
Tests failed: 0

*****************************

speth · 2020-03-24T01:09:15Z

Given the increased rate of releases of SUNDIALS, and the fact that we haven't seen breaking changes (at least, that affect Cantera) in several recent minor releases, I think we should assume compatibility with at least minor versions, i.e. 5.x. A build-time warning for versions newer than the latest we have tested would probably still be useful.

bryanwweber · 2020-03-24T02:56:06Z

Also, since we now support every released version back to 2.4, I think we should turn this into a LooseVersion check, rather than explicitly listing all of the satisfactory values. That will simplify that rather unwieldy line, I think. It could be something like

if LooseVersion(env['sundials_version']) >= LooseVersion('6.0'):
    error...

band-a-prend · 2020-03-25T07:59:17Z

@bryanwweber,
I replaced version list with LooseVersion conditions

bryanwweber

Couple of small changes

bryanwweber · 2020-03-26T01:08:24Z

SConstruct

@@ -1048,6 +1048,7 @@ env['has_demangle'] = conf.CheckDeclaration("boost::core::demangle",
                                '#include <boost/core/demangle.hpp>', 'C++')

 import SCons.Conftest, SCons.SConf
+from distutils.version import LooseVersion


You don't need to import this, LooseVersion is imported somewhere in the builtutils, which is * imported in SConstruct.

bryanwweber · 2020-03-26T01:10:12Z

SConstruct

@@ -1108,7 +1109,7 @@ if env['system_sundials'] == 'y':

    # Ignore the minor version, e.g. 2.4.x -> 2.4
    env['sundials_version'] = '.'.join(sundials_version.split('.')[:2])
-    if env['sundials_version'] not in ('2.4','2.5','2.6','2.7','3.0','3.1','3.2','4.0','4.1','5.0'):
+    if LooseVersion(env['sundials_version']) < LooseVersion('2.4') or LooseVersion(env['sundials_version']) >= LooseVersion('6.0'):


I'd suggest creating a variable for LooseVersion(env['sundials_version']) here to shorten this line... the whole point was to make it shorter 😂 You could then reuse that variable on line 1118 in that other if statement comparing to 2.4.

I think I then also could safely reuse this shorter variable at line 1610 because it's within another one if env['system_sundials'] == 'y' condition check.

system_sundials_version seems as reasonable name this case: although this will not make expression significantly shorter but it will avoid evaluation of LooseVersion() again and again for if env['system_sundials'] == 'y' blocks.

the whole point was to make it shorter 😂

Yes, at this point of view this attempt certainly has failed. But it then should become more readable in comparison with rapidly expanding list of numbers.

The last commit is updated.

bryanwweber · 2020-03-26T22:05:30Z

SConstruct

@@ -1108,13 +1108,14 @@ if env['system_sundials'] == 'y':

    # Ignore the minor version, e.g. 2.4.x -> 2.4
    env['sundials_version'] = '.'.join(sundials_version.split('.')[:2])
-    if env['sundials_version'] not in ('2.4','2.5','2.6','2.7','3.0','3.1','3.2','4.0','4.1','5.0'):
+    system_sundials_version = LooseVersion(env['sundials_version'])


This isn't necessarily associated with the system SUNDIALS though, so I think just sundials_ver would be sufficient and shorter

…on check

We expect that minor version number release will not introduce breaking changes, but the user should be warned that their version of SUNDIALS is untested.

codecov · 2020-03-27T18:19:19Z

Codecov Report

Merging #814 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #814   +/-   ##
=======================================
  Coverage   71.39%   71.39%           
=======================================
  Files         372      372           
  Lines       43482    43482           
=======================================
  Hits        31045    31045           
  Misses      12437    12437

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c16c4b0...846a7a6. Read the comment docs.

Simple fix for Sundials 5.1 compatibility

d347026

The Sundials 5.1 has no any changes in CVODES API in comparison with sundials 4.0, 4.1, 5.0 therefore the compatibility configuration fix is trivial.

band-a-prend added 2 commits March 4, 2020 09:13

Update Sundials submodule sha1 (tag v5.1.0)

b11fb2c

Update Sundials submodule info to Sundials 5.1.0

f696b48

bryanwweber requested changes Mar 26, 2020

View reviewed changes

band-a-prend force-pushed the sundials branch from 0c61485 to be9ff99 Compare March 26, 2020 07:11

bryanwweber requested changes Mar 26, 2020

View reviewed changes

Replace sundials version list with LooseVerion condition within versi…

fee586d

…on check

band-a-prend force-pushed the sundials branch from be9ff99 to fee586d Compare March 26, 2020 22:31

bryanwweber approved these changes Mar 26, 2020

View reviewed changes

[SCons] Warn if untested SUNDIALS is used

846a7a6

We expect that minor version number release will not introduce breaking changes, but the user should be warned that their version of SUNDIALS is untested.

bryanwweber added Input Input parsing and conversion (for example, ck2yaml) help wanted and removed Input Input parsing and conversion (for example, ck2yaml) help wanted labels Apr 2, 2020

speth approved these changes Apr 3, 2020

View reviewed changes

speth merged commit 3a9f637 into Cantera:master Apr 3, 2020

band-a-prend deleted the sundials branch September 27, 2020 00:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simple fix for Sundials 5.1 compatibility #814

Simple fix for Sundials 5.1 compatibility #814

band-a-prend commented Feb 28, 2020

band-a-prend commented Feb 29, 2020

bryanwweber commented Mar 3, 2020 •

edited

Loading

band-a-prend commented Mar 4, 2020 •

edited

Loading

bryanwweber commented Mar 4, 2020

band-a-prend commented Mar 4, 2020

band-a-prend commented Mar 4, 2020 •

edited

Loading

speth commented Mar 24, 2020

bryanwweber commented Mar 24, 2020

band-a-prend commented Mar 25, 2020

bryanwweber left a comment

bryanwweber Mar 26, 2020

band-a-prend Mar 26, 2020

bryanwweber Mar 26, 2020

band-a-prend Mar 26, 2020 •

edited

Loading

band-a-prend Mar 26, 2020

bryanwweber Mar 26, 2020

band-a-prend Mar 26, 2020

codecov bot commented Mar 27, 2020

Simple fix for Sundials 5.1 compatibility #814

Simple fix for Sundials 5.1 compatibility #814

Conversation

band-a-prend commented Feb 28, 2020

band-a-prend commented Feb 29, 2020

bryanwweber commented Mar 3, 2020 • edited Loading

band-a-prend commented Mar 4, 2020 • edited Loading

bryanwweber commented Mar 4, 2020

band-a-prend commented Mar 4, 2020

band-a-prend commented Mar 4, 2020 • edited Loading

speth commented Mar 24, 2020

bryanwweber commented Mar 24, 2020

band-a-prend commented Mar 25, 2020

bryanwweber left a comment

Choose a reason for hiding this comment

bryanwweber Mar 26, 2020

Choose a reason for hiding this comment

band-a-prend Mar 26, 2020

Choose a reason for hiding this comment

bryanwweber Mar 26, 2020

Choose a reason for hiding this comment

band-a-prend Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

band-a-prend Mar 26, 2020

Choose a reason for hiding this comment

bryanwweber Mar 26, 2020

Choose a reason for hiding this comment

band-a-prend Mar 26, 2020

Choose a reason for hiding this comment

codecov bot commented Mar 27, 2020

Codecov Report

bryanwweber commented Mar 3, 2020 •

edited

Loading

band-a-prend commented Mar 4, 2020 •

edited

Loading

band-a-prend commented Mar 4, 2020 •

edited

Loading

band-a-prend Mar 26, 2020 •

edited

Loading