Prevent errors due to unsafe math optimizations

[speth]

This is my proposed resolution to #1155.

Changes proposed in this pull request

• Require a working implementation of std::isnan, and suggest appropriate compiler flags if this is not the case
• Adjust test tolerances so that running with -ffast-math -fno-finite-math-only passes the test suite
• Add these flags to one set of CI builds (I added this to the "multiple Sundials" runs somewhat arbitrarily)

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

Closes #1155

Checklist

• The pull request includes a clear description of this code change
• Commit messages have short titles and reference relevant issues
• Build passes (scons build & scons test) and unit tests address code coverage
• Style & formatting of contributed code follows contributing guidelines
• The pull request is ready for review

[ischoegl]

Thank for addressing #1155! This looks mostly good to me, and should take care of most concerns from #1155 (I still think that NaN should be avoided long-term, as not all compilers offer granular control over fast-math options). As an error is in place, closing #1155 via this PR is 👍.

Regarding self.assertNear vs. self.assertEqual: equivalent changes probably should be applied across the test suite whenever floats are compared? (which would be a lot of churn, so I'm not exactly advocating for this here.) At the same time, the issues probably only arise here due to the fact that activation energy input requires more processing than other items.

[speth]

Regarding self.assertNear vs. self.assertEqual: equivalent changes probably should be applied across the test suite whenever floats are compared? (which would be a lot of churn, so I'm not exactly advocating for this here.) At the same time, the issues probably only arise here due to the fact that activation energy input requires more processing than other items.

While it's not obvious from the diff alone, the instances I changed are more than just the test cases that failed -- these are the other instances in at least these Python tests where a calculated value was being compared for exact equality (because we go back and forth between Ea/R and E). I didn't go through kineticsFromYaml.cpp as thoroughly.

[ischoegl]

I didn't go through kineticsFromYaml.cpp as thoroughly.

I think those discrepancies of exact bit-wise matches can occur anytime a unit conversion happens. I'm not advocating for large-scale changes (if any beyond what you already changed) in this PR, but it may be safer to avoid exact comparisons for floating point values in general?

[speth]

it may be safer to avoid exact comparisons for floating point values in general?

Of course, and I think in general, we do a good job of that. There are just a lot comparisons in the test suite, and it's easy to type ASSERT_EQ when you mean ASSERT_NEAR (and also have to decide how near you mean).

[ischoegl]

it's easy to type ASSERT_EQ when you mean ASSERT_NEAR

... guilty as charged! Certainly has happened in some of the tests I wrote.

[codecov]

Codecov Report

Merging #1161 (71a0ffa) into main (80c1e56) will increase coverage by 0.00%.
The diff coverage is 66.66%.

@@           Coverage Diff           @@
##             main    #1161   +/-   ##
=======================================
  Coverage   66.10%   66.10%           
=======================================
  Files         313      313           
  Lines       44963    44955    -8     
  Branches    19144    19137    -7     
=======================================
- Hits        29721    29719    -2     
+ Misses      12658    12652    -6     
  Partials     2584     2584           

Impacted Files	Coverage Δ
src/equil/vcs_solve_TP.cpp	61.69% <66.66%> (+0.15%)	⬆️

---

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 80c1e56...71a0ffa. Read the comment docs.

[ischoegl]

Thank you, this looks good to me!