Wrap up GasKinetics #1101

ischoegl · 2021-09-17T22:12:38Z

Changes proposed in this pull request

Introduce BlowersMaselReaction3 (and associated BlowersMaselRate) to new framework
Introduce FalloffReaction3 and associated reaction rate handlers for pre-existing Falloff, Troe, SRI, and Tsang
Introduce a new Arrhenius.h header that collects reaction rates for Arrhenius3, BlowersMasel3 and potential future additions (e.g. ElectronTemperature in electron-temperature reaction #1099) in analogy to Falloff.h.
Simplify handling of reaction rate units (new framework stores Units with rate objects)
Make shared ReactionData more flexible by providing Kinetics references to update methods
Clarify interaction with ThirdBody and ThirdbodyCalc
ChemicallyActivatedReaction are treated as a special case of FalloffReaction3

New FalloffReaction3 and associated rate objects bypass FalloffFactory.h and FalloffMgr, which at this point are only used by legacy implementations.

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

Addresses Cantera/enhancements#87

If applicable, provide an example illustrating new features this pull request is introducing

A simple benchmark for YAML (new framework) vs XML (legacy objects) implies a slight overall speedup:

In [4]: %run custom_reactions.py
Average time of 100 simulation runs for 'gri30.yaml' (CH4)
- New framework (YAML): 157.51 ms (T_final=2736.60)
- One Python reaction: 179.26 ms (T_final=2736.60) ... +13.81%
- Old framework (XML): 161.23 ms (T_final=2736.60) ... +2.36%

(for comparison, the current main shows YAML tests about 1% faster)

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

codecov · 2021-09-17T22:45:25Z

Codecov Report

Merging #1101 (c357871) into main (0805854) will increase coverage by 0.10%.
The diff coverage is 90.24%.

❗ Current head c357871 differs from pull request most recent head 3a662b3. Consider uploading reports for the commit 3a662b3 to get more accurate results

@@            Coverage Diff             @@
##             main    #1101      +/-   ##
==========================================
+ Coverage   73.63%   73.73%   +0.10%     
==========================================
  Files         366      370       +4     
  Lines       48373    48927     +554     
==========================================
+ Hits        35619    36077     +458     
- Misses      12754    12850      +96

Impacted Files	Coverage Δ
include/cantera/base/global.h	`93.33% <ø> (ø)`
include/cantera/base/utilities.h	`100.00% <ø> (ø)`
include/cantera/kinetics/GasKinetics.h	`100.00% <ø> (ø)`
include/cantera/kinetics/Kinetics.h	`55.26% <0.00%> (-1.50%)`	⬇️
include/cantera/kinetics/ReactionRateFactory.h	`100.00% <ø> (ø)`
include/cantera/kinetics/MultiRate.h	`72.00% <75.60%> (-10.61%)`	⬇️
include/cantera/kinetics/ReactionRate.h	`75.60% <75.60%> (+14.27%)`	⬆️
src/kinetics/GasKinetics.cpp	`85.71% <77.77%> (-6.90%)`	⬇️
include/cantera/kinetics/RxnRates.h	`80.74% <79.41%> (-11.17%)`	⬇️
src/base/AnyMap.cpp	`90.56% <80.00%> (-0.26%)`	⬇️
... and 43 more

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 0805854...3a662b3. Read the comment docs.

speth

Thanks for the updates, @ischoegl. Assuming the latest round of tests pass, I think this is ready to merge.

ischoegl · 2021-12-02T02:41:13Z

🎉 ... I just ran a test with stock ubuntu plus the cython pre-release, which is an environment I can reproduce locally. However, this failed also, and created an additional failure (!). So I'll drop 0de6bee ...

For flame simulations that don't use the 'eField' solution component, replace the default solution bounds which require this component to remain exactly zero with arbitrarily wide bounds. Small rounding errors can occasionally cause the solver to want to have this component take on a small nonzero value, which could not be satisfied for any step size, resulting in solver failure.

ischoegl · 2021-12-02T02:45:51Z

@speth ... looks like we ended up pushing attempts to fix at the same time; mine definitely didn't work and may have compromised your fix. Dropped my commit and force-pushed, so we should be back to what you tested locally. Crossing fingers that this may finally work. Btw, I truly appreciate you investigating this 'interesting' failure!

ischoegl · 2021-12-02T03:15:06Z

@speth ... too bad - CI still fails. I recompiled on my local ubuntu 20.04+latest cython docker image, and still get:

$ scons test-python-onedim
[...]
==================================================================== 83 passed in 18.76s =====================================================================
scons: done building targets.
$ ipython
Python 3.8.10 (default, Sep 28 2021, 16:10:42) 
Type 'copyright', 'credits' or 'license' for more information
IPython 7.29.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import cython

In [2]: cython.__version__
Out[2]: '3.0.0a9'

which is the exact same setup that I just tried on GH actions (with resulted in two failures). Running on a 10th gen i7 here; have a Xeon on my desktop, which I can try tomorrow. How were you able to reproduce locally?

PS: to recap when this started … the first time I saw this failure was when I rebased and adopted your proposed changes (which I pushed at the same time). It’s really hard to pinpoint, as it could have been introduced by any of 70+ commits.

speth · 2021-12-02T04:11:25Z

Whoops, look like only tested with my partial extract of the failing test, not with the full test suite. I think my fix did resolve the original problem, which was leading to a 1D solver failure after taking too many timesteps. The error now is different:

*******************************************************************************
InputFileError thrown by AnyValue::as:
Error on line 287 of /home/runner/work/cantera/cantera/test/work/python/onedim-add-species.yaml:
Key '' contains a 'string',
not a 'double'
|  Line |
|   282 |     9.150408995970660e-111, 9.096486800225543e-111, 9.004665021489618e-111,
|   283 |     8.946000916505259e-111, 8.883492076218488e-111, 8.819368872054378e-111,
|   284 |     8.757793143699390e-111, 8.647460929553677e-111, 8.548463735480379e-111,
|   285 |     8.454994524806244e-111, 8.365084705512343e-111, 8.279866258720111e-111,
|   286 |     8.201057565230892e-111, 8.129435107436128e-111, 8.007349868937955e-111,
>   287 >     7.907764574584298e-111, 7.8200590543282ee-111, 7.647334294426071e-111,
                                      ^
|   288 |     7.448401111118985e-111, 6.912720580779952e-111, 6.157719215668146e-111,
|   289 |     5.185696447574553e-111, 2.671273012561201e-111, -1.181310723018686e-112,
|   290 |     -2.268848055163603e-111, -1.387197256686392e-111, 3.071316259129547e-110,
*******************************************************************************

Is there an issue with the new double formatter that could cause this doubled e? I was able to replicate this locally, so can take a look into it tomorrow if you don't have a configuration that exhibits this behavior.

ischoegl · 2021-12-02T04:21:50Z

This indeed looks like something that would have been introduced by #1133.

ischoegl · 2021-12-02T05:26:09Z

@speth ... this helps! Yup, there was a pretty obvious glitch in formatDouble, which inexplicably didn't show up in any of the other testing: it boils down to having assumed that the exponent has a fixed length, which - while mostly true - is not true for very small or very large numbers. Knowing what to look for, I was able to reproduce locally and fix (and also add this to the unit test). Also fixed a minor papercut with roundf vs round in Units where I noticed some compiler warnings in unrelated testing.

Thanks for locating the relevant error! So ... this may be ready!

Previous implementation assumed number format with four letter exponent blocks; this fix ensures that formatting is correct for very small/large numbers that use five letter exponent blocks (e.g. 'e+100')

ischoegl · 2021-12-02T15:53:14Z

Force-pushed an update that should have less of an impact on performance, while still fixing the glitch in formatDouble ...

speth

Looks good to me. Time to merge!

decaluwe · 2021-12-02T17:43:25Z

🎉

…

On Dec 2, 2021, at 10:40 AM, Ray Speth ***@***.***> wrote: @speth approved this pull request. Looks good to me. Time to merge! — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1101 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABEC7PEEGDD2JZO7DRDZE23UO6VRVANCNFSM5EINUFJQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

ischoegl · 2021-12-02T17:45:46Z

Thanks, @speth for your patience in reviewing as well as the assistance in tracking some of the failures down. Much appreciated!

bryanwweber · 2021-12-02T18:07:44Z

Thanks @ischoegl for taking this on and @speth for reviewing!

ischoegl force-pushed the wrap-up-gaskinetics branch from dd6d7ab to 006c24a Compare September 17, 2021 22:54

ischoegl mentioned this pull request Sep 18, 2021

Refactor Reaction Rate Evaluators Cantera/enhancements#87

Closed

ischoegl force-pushed the wrap-up-gaskinetics branch 17 times, most recently from dda597f to ed91060 Compare September 23, 2021 03:54

ischoegl added the Kinetics label Sep 23, 2021

ischoegl force-pushed the wrap-up-gaskinetics branch 6 times, most recently from 87e3185 to 1987326 Compare October 7, 2021 19:25

ischoegl mentioned this pull request Oct 8, 2021

electron-temperature reaction #1099

Merged

5 tasks

ischoegl force-pushed the wrap-up-gaskinetics branch 2 times, most recently from 5deeade to b16128e Compare October 8, 2021 21:34

speth previously approved these changes Dec 2, 2021

View reviewed changes

ischoegl dismissed speth’s stale review via ab8fd9f December 2, 2021 02:42

ischoegl force-pushed the wrap-up-gaskinetics branch from 8d05d42 to ab8fd9f Compare December 2, 2021 02:42

[Units] Replace roundf by round

1b46a24

ischoegl force-pushed the wrap-up-gaskinetics branch 3 times, most recently from e34aec2 to 11b30ac Compare December 2, 2021 05:02

ischoegl requested a review from speth December 2, 2021 05:26

ischoegl force-pushed the wrap-up-gaskinetics branch from 11b30ac to 1905f96 Compare December 2, 2021 15:01

[YAML] Fix glitch in formatDouble serialization

3a662b3

Previous implementation assumed number format with four letter exponent blocks; this fix ensures that formatting is correct for very small/large numbers that use five letter exponent blocks (e.g. 'e+100')

ischoegl force-pushed the wrap-up-gaskinetics branch from 1905f96 to 3a662b3 Compare December 2, 2021 15:04

speth approved these changes Dec 2, 2021

View reviewed changes

speth merged commit ad213c4 into Cantera:main Dec 2, 2021

ischoegl deleted the wrap-up-gaskinetics branch December 2, 2021 17:45

This was referenced Dec 7, 2021

Modify third body efficiencies using modify_reaction #1143

Open

Update ReactionData and consolidate update strategies #1153

Merged

speth mentioned this pull request Dec 17, 2021

[Kinetics] Fix issue 717, Add check for sticking coefficient more than 1 #1038

Merged

5 tasks

ischoegl mentioned this pull request Dec 29, 2021

Bump version to 2.6.0a4 #1163

Merged

5 tasks

ischoegl mentioned this pull request Jan 6, 2022

Proof-of-concept: Modify reaction rates in memory #1051

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrap up GasKinetics #1101

Wrap up GasKinetics #1101

ischoegl commented Sep 17, 2021 •

edited

Loading

codecov bot commented Sep 17, 2021 •

edited

Loading

speth left a comment

ischoegl commented Dec 2, 2021

ischoegl commented Dec 2, 2021

ischoegl commented Dec 2, 2021 •

edited

Loading

speth commented Dec 2, 2021

ischoegl commented Dec 2, 2021

ischoegl commented Dec 2, 2021 •

edited

Loading

ischoegl commented Dec 2, 2021

speth left a comment

decaluwe commented Dec 2, 2021 via email

ischoegl commented Dec 2, 2021

bryanwweber commented Dec 2, 2021

Wrap up GasKinetics #1101

Wrap up GasKinetics #1101

Conversation

ischoegl commented Sep 17, 2021 • edited Loading

codecov bot commented Sep 17, 2021 • edited Loading

Codecov Report

speth left a comment

Choose a reason for hiding this comment

ischoegl commented Dec 2, 2021

ischoegl commented Dec 2, 2021

ischoegl commented Dec 2, 2021 • edited Loading

speth commented Dec 2, 2021

ischoegl commented Dec 2, 2021

ischoegl commented Dec 2, 2021 • edited Loading

ischoegl commented Dec 2, 2021

speth left a comment

Choose a reason for hiding this comment

decaluwe commented Dec 2, 2021 via email

ischoegl commented Dec 2, 2021

bryanwweber commented Dec 2, 2021

ischoegl commented Sep 17, 2021 •

edited

Loading

codecov bot commented Sep 17, 2021 •

edited

Loading

ischoegl commented Dec 2, 2021 •

edited

Loading

ischoegl commented Dec 2, 2021 •

edited

Loading