-
Notifications
You must be signed in to change notification settings - Fork 2
Adds Taylor Series rate constant type #116
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new Taylor Series rate constant type in the V1 configuration to support GECKO-A mechanisms by combining an Arrhenius term with a variable‐order Taylor series.
- Added the
TaylorSeriesstruct and integrated its parser - Created schema, unit tests, and CMake updates for validation
- Updated documentation to describe the new reaction format
Reviewed Changes
Copilot reviewed 20 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/v1/v1_unit_configs/reactions/taylor_series/valid.yaml, valid.json | Add valid Taylor series configuration tests |
| test/unit/v1/v1_unit_configs/reactions/taylor_series/unknown_species.yaml, .json | Add tests for unknown species error |
| test/unit/v1/v1_unit_configs/reactions/taylor_series/mutually_exclusive.yaml, .json | Add tests for C vs. Ea mutual exclusivity |
| test/unit/v1/v1_unit_configs/reactions/taylor_series/missing_phase.yaml, .json | Add tests for missing phase error |
| test/unit/v1/v1_unit_configs/reactions/taylor_series/bad_reaction_component.yaml, .json | Add tests for invalid reaction component keys |
| test/unit/v1/reactions/test_parse_taylor_series.cpp | Implement parser unit tests for Taylor series |
| test/unit/v1/reactions/CMakeLists.txt | Register Taylor series test target |
| src/v1/reactions/taylor_series_parser.cpp | Implement the TaylorSeriesParser logic |
| src/v1/reactions/CMakeLists.txt | Include parser source in build |
| src/v1/mechanism_parsers.cpp | Register TaylorSeriesParser in mechanism parser |
| include/mechanism_configuration/v1/validation.hpp | Define schema keys for TAYLOR_SERIES and keys |
| include/mechanism_configuration/v1/reaction_types.hpp | Define the TaylorSeries reaction type |
| include/mechanism_configuration/v1/reaction_parsers.hpp | Declare the TaylorSeriesParser interface |
| docs/source/reactions/taylor_series.rst | Add documentation for Taylor series reactions |
| docs/source/reactions/index.rst | Add taylor_series entry to reactions index |
Comments suppressed due to low confidence (3)
test/unit/v1/reactions/test_parse_taylor_series.cpp:76
- Add a unit test for a reaction that specifies only
Ea(withoutC) to verify that the parser correctly convertsEaintoCusing the Boltzmann constant.
}
docs/source/reactions/taylor_series.rst:45
- The YAML example shows
reactantsandproductsas mappings (e.g.,spec1:), but the parser expects a sequence of objects withspecies nameand optionalcoefficient. Update the example to match the actual list-based format.
reactants:
docs/source/reactions/taylor_series.rst:44
- The example includes a
time unitfield which is not parsed by the TaylorSeries parser. Either remove it from the sample or document its handling explicitly.
time unit: MIN
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #116 +/- ##
=======================================
Coverage 86.95% 86.95%
=======================================
Files 3 3
Lines 23 23
=======================================
Hits 20 20
Misses 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
boulderdaze
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I just noticed that some files don't have any content in them but everything looks good.
Adds a new rate constant type to the V1 configuration. This type is needed to support GECKO-A mechanisms. I have asked for references for this equation, but haven't got them yet. If they arrive in time, I can add them to this PR, otherwise I can submit a follow-up PR. The rate constant is calculated as an Arrhenius reactions multiplied by a variable order Taylor series based on temperature.