Fix logic error in CanteraTest.assertNear #1120
Conversation
The behavior of collecting errors related to trying to add all reactions before reporting them can make it hard to diagnose some problems. Since disabling optimizations is usually an early step in the debugging process, this will make such problems apparent at that time.
ctml2yamlTest.test_water_IAPWS95_thermo was trying to calculate transport properties in a region where the underlying model (which assumes liquid water) wasn't valid, resulting in NaNs.
Using chained getter/setter notation to set the rate of a Plog reaction doesn't work because the 'rate' getter always returns a copy of the PlogRate object, and modifying this object doesn't update the parent Reaction object.
Codecov Report
@@ Coverage Diff @@
## main #1120 +/- ##
=======================================
Coverage 73.45% 73.45%
=======================================
Files 364 364
Lines 47882 47884 +2
=======================================
+ Hits 35170 35172 +2
Misses 12712 12712
Continue to review full report at Codecov.
|
| @@ -70,7 +70,7 @@ def tearDownClass(cls): | |||
|
|
|||
| def assertNear(self, a, b, rtol=1e-8, atol=1e-12, msg=None): | |||
| cmp = 2 * abs(a - b)/(abs(a) + abs(b) + 2 * atol / rtol) | |||
| if cmp > rtol: | |||
| if not cmp < rtol: | |||
There was a problem hiding this comment.
I can never remember the priority order of not versus comparisons. I think it'd make sense to group the comparison for clarity, although it's not required. Also if you can add a comment here as to why the comparison is done this way, I think that'd help too. The previous version is more intuitive.
There was a problem hiding this comment.
Oops - should have waited longer before merging. I thought the logic made sense. I can add the comment in #1101.
Changes proposed in this pull request
This fixes an issue I found while trying to explore the changes in #1101 where I realized that some problems weren't getting caught by the tests even though they were clearly covered by the test cases.
assertNearandassertArrayNearfunctions that causedinfandnaninputs to unexpectedly passPlus two more minor updates:
Transport::CKModein Cython moduleChecklist
scons build&scons test) and unit tests address code coverage