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.

Sign up for GitHub

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

Jump to bottom

Add support for SUNDIALS 5.5-5.7 compiled with LAPACK #1004

Merged
merged 5 commits into from
Apr 9, 2021
Merged

Add support for SUNDIALS 5.5-5.7 compiled with LAPACK #1004

merged 5 commits into from
Apr 9, 2021

Conversation

bryanwweber
Copy link
Member

@bryanwweber bryanwweber commented Mar 31, 2021 •
edited
Loading

Changes proposed in this pull request

  • Support for system SUNDIALS 5.5-5.7 with LAPACK. The preprocessor define indicating LAPACK support in SUNDIALS was changed with version 5.5.
  • Fix that the Boost version message is printed even if Boost isn't found
  • Simplify preprocessor logic in BandMatrix.cpp
  • Fix deprecation warning about SunLinSol_SPGMR
  • Fix error when omp.h is installed, but the library cannot be found by the linker (for example, omp installed by Homebrew, the Homebrew path on the search path, but using Apple's compilers).

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

@bryanwweber bryanwweber changed the title Fixes Add support for SUNDIALS 5.5-5.7 compiled with LAPACK Apr 1, 2021
@codecov
Copy link

codecov bot commented Apr 2, 2021 •
edited
Loading

Codecov Report

Merging #1004 (929ffb7) into main (3803972) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

❗ Current head 929ffb7 differs from pull request most recent head e379b6d. Consider uploading reports for the commit e379b6d to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1004      +/-   ##
==========================================
- Coverage   70.67%   70.66%   -0.02%     
==========================================
  Files         362      361       -1     
  Lines       44554    44522      -32     
==========================================
- Hits        31488    31460      -28     
+ Misses      13066    13062       -4
Impacted Files Coverage Δ
src/numerics/BandMatrix.cpp 61.13% <ø> (ø)
src/numerics/CVodesIntegrator.cpp 74.39% <0.00%> (ø)
src/zeroD/ReactorNet.cpp 84.50% <0.00%> (-0.50%) ⬇️
samples/cxx/openmp_ignition/openmp_ignition.cpp

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 3803972...e379b6d. Read the comment docs.

@bryanwweber bryanwweber marked this pull request as ready for review April 3, 2021 15:12
@bryanwweber
Copy link
Member Author

bryanwweber commented Apr 3, 2021 •
edited
Loading

By the way, I think it'd be good to backport at least 64c0736 fc93ece to the 2.5 branch

@bryanwweber bryanwweber requested a review from a team April 4, 2021 00:46
ischoegl
ischoegl previously approved these changes Apr 7, 2021
View reviewed changes
Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

Per my comments elsewhere, this isn't necessarily my strong suit (and I don't have the toolchains to verify this installed). On the other hand, both PR rationale and code comments are clear and the changes appear to be straightforward. Testing every combination in GH Actions may be overkill, but if there are candidates of particular concern this may be warranted. Overall, no concerns from my side ...

src/numerics/BandMatrix.cpp Show resolved Hide resolved
speth
speth reviewed Apr 8, 2021
View reviewed changes
Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

This looks good from a functional standpoint. I just have a couple of requests on the formatting.

SConstruct Outdated Show resolved Hide resolved
@bryanwweber
Simplify header includes in BandMatrix
7c75c20 
If CT_USE_LAPACK is false, then CT_SUNDIALS_USE_LAPACK is also always
false. Thus the CT_SUNDIALS_USE_LAPACK branch here was never being
exercised.
@bryanwweber
[SCons] Fix LAPACK defines for SUNDIALS
fc93ece 
In SUNDIALS >= 5.5, there is no longer a define that indicates SUNDIALS
was compiled with LAPACK globally. Instead, the specific SUNLINSOL
libraries that actually use LAPACK are listed explicitly.
@bryanwweber
[SCons] Don't print Boost info if it isn't found
a593c75
@bryanwweber
Fix compiler warning with more recent SUNDIALS
ae80850 
SUNDIALS 4.0 deprecated the SUNSPGMR function name, replacing it with
a more specific version. The more specific version isn't available in
SUNDIALS 3.0 though, so we need another check here.
@bryanwweber
[SCons/Samples] Fix finding OMP library
e379b6d 
It is possible for the OMP header to be installed in the search
path, but the library not to be available. This can happen, for
example on macOS when Boost and GCC are installed by Homebrew,
the Homebrew include path is set to find Boost, and the Apple
compilers are being used.
@bryanwweber bryanwweber requested a review from speth April 9, 2021 13:18
@speth speth merged commit 3d672b0 into Cantera:main Apr 9, 2021
@bryanwweber bryanwweber deleted the update-sundials branch April 9, 2021 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ischoegl ischoegl ischoegl left review comments

@speth speth speth approved these changes

Assignees
No one assigned
Labels
compiling
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bryanwweber @speth @ischoegl