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

Fix logic error in CanteraTest.assertNear #1120

Merged
merged 7 commits into from
Oct 17, 2021
Merged

Fix logic error in CanteraTest.assertNear #1120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
79e7628
[Kinetics] Fail fast when adding reactions with optimizations disabled
speth Oct 16, 2021
cca11fe
[Python/Transport] Fix handling of exceptions from CKMode
speth Oct 16, 2021
eb7a7db
[Test] Use valid conditions for checking liquid water properties
speth Oct 16, 2021
0d82d90
[Test] Add workaround for Plog rate setting test
speth Oct 17, 2021
14213c1
[Kinetics] Fix setting parameters of standalone Arrhenius rates
speth Oct 17, 2021
c23fe0b
[Test] Check that unassigned reaction rates return NaN
speth Oct 17, 2021
421bb7a
[Test] Fix assertNear to handle inf and nan correctly
speth Oct 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion interfaces/cython/cantera/_cantera.pxd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,7 +614,7 @@ cdef extern from "cantera/transport/TransportBase.h" namespace "Cantera":
cdef cppclass CxxTransport "Cantera::Transport":
CxxTransport(CxxThermoPhase*)
string transportType()
cbool CKMode()
cbool CKMode() except +translate_exception
double viscosity() except +translate_exception
double thermalConductivity() except +translate_exception
double electricalConductivity() except +translate_exception
Expand Down
1 change: 1 addition & 0 deletions interfaces/cython/cantera/test/test_convert.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1150,6 +1150,7 @@ def test_water_IAPWS95_thermo(self):
ctmlWater, yamlWater = self.checkConversion("liquid-water")
self.checkThermo(ctmlWater, yamlWater, [300, 500, 1300, 2000], pressure=22064000.0)
self.assertEqual(ctmlWater.transport_model, yamlWater.transport_model)
ctmlWater.TP = yamlWater.TP = 300, 22064000.0
dens = ctmlWater.density
for T in [298, 1001, 2400]:
ctmlWater.TD = T, dens
Expand Down
5 changes: 4 additions & 1 deletion interfaces/cython/cantera/test/test_kinetics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1121,12 +1121,15 @@ def test_plog(self):
r = ct.PlogReaction()
r.reactants = {'R1A':1, 'R1B':1}
r.products = {'P1':1, 'H':1}
r.rate.rates = [
# @todo: Fix so chained setter works, i.e. r.rate.rates = ...
rate = r.rate
rate.rates = [
(0.01*ct.one_atm, ct.Arrhenius(1.2124e13, -0.5779, 10872.7*4184)),
(1.0*ct.one_atm, ct.Arrhenius(4.9108e28, -4.8507, 24772.8*4184)),
(10.0*ct.one_atm, ct.Arrhenius(1.2866e44, -9.0246, 39796.5*4184)),
(100.0*ct.one_atm, ct.Arrhenius(5.9632e53, -11.529, 52599.6*4184))
]
r.rate = rate

gas2 = ct.Solution(thermo='IdealGas', kinetics='GasKinetics',
species=species, reactions=[r])
Expand Down
14 changes: 7 additions & 7 deletions interfaces/cython/cantera/test/test_reaction.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,17 +477,17 @@ def test_no_rate(self):
return
rxn = self._cls(equation=self._equation, kinetics=self.gas,
legacy=self._legacy, **self._kwargs)
if self._legacy:
self.assertNear(rxn.rate(self.gas.T), 0.)
else:
self.assertTrue(np.isnan(rxn.rate(self.gas.T, self.gas.P)))

gas2 = ct.Solution(thermo="IdealGas", kinetics="GasKinetics",
species=self.species, reactions=[rxn])
gas2.TPX = self.gas.TPX

self.assertNear(gas2.forward_rate_constants[0], 0.)
self.assertNear(gas2.net_rates_of_progress[0], 0.)
if self._legacy:
self.assertNear(rxn.rate(self.gas.T), 0.)
self.assertNear(gas2.forward_rate_constants[0], 0.)
self.assertNear(gas2.net_rates_of_progress[0], 0.)
else:
self.assertTrue(np.isnan(rxn.rate(self.gas.T, self.gas.P)))
self.assertTrue(np.isnan(gas2.forward_rate_constants[0]))

def test_replace_rate(self):
# check replacing reaction rate expression
Expand Down
6 changes: 3 additions & 3 deletions interfaces/cython/cantera/test/utilities.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Oops - should have waited longer before merging. I thought the logic made sense. I can add the comment in #1101.

message = ('AssertNear: %.14g - %.14g = %.14g\n' % (a, b, a-b) +
'Relative error of %10e exceeds rtol = %10e' % (cmp, rtol))
if msg:
Expand All @@ -88,12 +88,12 @@ def assertArrayNear(self, A, B, rtol=1e-8, atol=1e-12, msg=None):
a = A[i]
b = B[i]
cmp = 2 * abs(a - b)/(abs(a) + abs(b) + 2 * atol / rtol)
if cmp > rtol:
if not cmp < rtol:
message = ('AssertNear: {:.14g} - {:.14g} = {:.14g}\n'.format(a, b, a-b) +
'Relative error for element {} of {:10e} exceeds rtol = {:10e}'.format(i, cmp, rtol))
if msg:
message = msg + '\n' + message
if cmp > worst[0]:
if not cmp < worst[0]:
worst = cmp, message

if worst[0]:
Expand Down
12 changes: 8 additions & 4 deletions src/kinetics/KineticsFactory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,11 +183,15 @@ void addReactions(Kinetics& kin, const AnyMap& phaseNode, const AnyMap& rootNode
} else {
// specified section is in the current file
for (const auto& R : rootNode.at(sections[i]).asVector<AnyMap>()) {
try {
#ifdef NDEBUG
try {
kin.addReaction(newReaction(R, kin), false);
} catch (CanteraError& err) {
format_to(add_rxn_err, "{}", err.what());
}
#else
kin.addReaction(newReaction(R, kin), false);
} catch (CanteraError& err) {
format_to(add_rxn_err, "{}", err.what());
}
#endif
}
}
}
Expand Down
15 changes: 10 additions & 5 deletions src/kinetics/RxnRates.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,19 @@ void Arrhenius::setParameters(const AnyValue& rate,
m_E = NAN;
} else if (rate.is<AnyMap>()) {
auto& rate_map = rate.as<AnyMap>();
if (rate_units.factor() == 0 && rate_map["A"].is<std::string>()) {
if (rate_units.factor() == 0) {
// A zero rate units factor is used as a sentinel to detect
// stand-alone reaction rate objects
throw InputFileError("Arrhenius::setParameters", rate_map,
"Specification of units is not supported for pre-exponential factor "
"when\ncreating a standalone 'ReactionRate' object.");
if (rate_map["A"].is<std::string>()) {
throw InputFileError("Arrhenius::setParameters", rate_map,
"Specification of units is not supported for pre-exponential "
"factor when\ncreating a standalone 'ReactionRate' object.");
} else {
m_A = rate_map["A"].asDouble();
}
} else {
m_A = units.convert(rate_map["A"], rate_units);
}
m_A = units.convert(rate_map["A"], rate_units);
m_b = rate_map["b"].asDouble();
m_E = units.convertActivationEnergy(rate_map["Ea"], "K");
} else {
Expand Down