Remove CT_SUNDIALS_VERSION constant in favor of SUNDIALS-defined constants#1746
Merged
speth merged 7 commits intoCantera:mainfrom Aug 1, 2024
Merged
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1746 +/- ##
==========================================
+ Coverage 73.04% 73.12% +0.08%
==========================================
Files 381 381
Lines 54164 54134 -30
Branches 9240 9222 -18
==========================================
+ Hits 39562 39587 +25
+ Misses 11633 11589 -44
+ Partials 2969 2958 -11 ☔ View full report in Codecov by Sentry. |
bryanwweber
requested review from
a team and
speth
and removed request for
speth
bryanwweber
force-pushed
the
bryan-remove-ct_sundials_version
branch
from
53ef06c to
2015e46
Compare
speth
reviewed
Jul 30, 2024
The CT_SUNDIALS_VERSION constant was originally defined when SUNDIALS only defined a string version number, so we couldn't do comparisons. Since SUNDIALS v3.0, the version parts have been available as integers, enabling comparison with the SUNDIALS-defined constants. This change simplifies building the libraries because it removes the custom constant and simplifies building the sdist/wheel because its one fewer constant that has to be templated in.
bryanwweber
force-pushed
the
bryan-remove-ct_sundials_version
branch
2 times, most recently
from
2271065 to
805b629
Compare
bryanwweber
commented
Jul 31, 2024
| #endif | ||
|
|
||
| #if CT_SUNDIALS_VERION >= 60 | ||
| #if SUNDIALS_VERSION_MAJOR >= 6 |
Member
Author
There was a problem hiding this comment.
By the way, this line had a typo before, so these lines were not being included, I believe.
bryanwweber
force-pushed
the
bryan-remove-ct_sundials_version
branch
from
805b629 to
4c25795
Compare
bryanwweber
force-pushed
the
bryan-remove-ct_sundials_version
branch
from
4c25795 to
96a7479
Compare
5 tasks
speth
approved these changes
Aug 1, 2024
Member
speth
left a comment
speth
left a comment
There was a problem hiding this comment.
Thanks, @bryanwweber. This looks good to me.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes proposed in this pull request
CT_SUNDIALS_VERSIONwithSUNDIALS_VERSION_MAJOR. In all the existing cases, the comparison was tox0(wherexwas one of 4, 6, or 7) indicating only the major version was being considered.If applicable, fill in the issue number this pull request is fixing
Closes Cantera/enhancements#209
If applicable, provide an example illustrating new features this pull request is introducing
Checklist
scons build&scons test) and unit tests address code coverage