Add in-code option for Python mechanisms#90
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an in‐code option for specifying chemical mechanisms in Python and updates the data model for reaction components.
- Changed tests to validate coefficients for species using a ReactionComponent instead of a plain string.
- Updated parser code in henrys_law and condensed_phase photolysis to construct ReactionComponent objects.
- Adjusted YAML configuration to reflect the new fields and structure.
Reviewed Changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/v1/test_parse_simpol_phase_transfer.cpp | Added coefficient checks for gas and aerosol species. |
| test/unit/v1/test_parse_henrys_law.cpp | Updated tests to confirm ReactionComponent fields for species. |
| test/unit/v1/test_parse_condensed_phase_photolysis.cpp | Updated scaling factor reference for photolysis reactions. |
| src/v1/henrys_law_parser.cpp | Refactored to build ReactionComponent objects for gas, aerosol, and water species. |
| src/v1/condensed_phase_photolysis_parser.cpp | Changed the scaling factor field to match the updated struct. |
| include/mechanism_configuration/v1/types.hpp | Modified member types: using ReactionComponent for species and renaming scaling_factor. |
| examples/v1/full_configuration.yaml | Revised reaction configuration keys and values to align with new schema. |
Files not reviewed (2)
- Dockerfile: Language not supported
- examples/v1/full_configuration.json: Language not supported
Comments suppressed due to low confidence (2)
src/v1/henrys_law_parser.cpp:90
- The coefficient for gas_component is not explicitly set and relies on default initialization; please verify that ReactionComponent defaults its coefficient to 1 to satisfy the test expectations.
gas_component.species_name = gas_phase_species;
examples/v1/full_configuration.yaml:249
- Double-check that the updated ARRHENIUS reaction configuration—specifically the ordering and the inclusion of both species name and coefficient in the reactants—matches the expected schema in the parser.
- species name: A
| henrys_law.aerosol_phase_species = aerosol_phase_species; | ||
| henrys_law.aerosol_phase_water = aerosol_phase_water; | ||
| types::ReactionComponent aerosol_component; | ||
| aerosol_component.species_name = aerosol_phase_species; |
There was a problem hiding this comment.
The coefficient for aerosol_component is not explicitly initialized; consider explicitly setting it to 1 to ensure consistency with the tests.
| aerosol_component.species_name = aerosol_phase_species; | |
| aerosol_component.species_name = aerosol_phase_species; | |
| aerosol_component.coefficient = 1; // Explicitly set coefficient to 1 for consistency |
| types::ReactionComponent aerosol_component; | ||
| aerosol_component.species_name = aerosol_phase_species; | ||
| henrys_law.aerosol_phase_species = aerosol_component; | ||
| types::ReactionComponent aerosol_phase_water_component; |
There was a problem hiding this comment.
The coefficient for aerosol_phase_water_component is also not explicitly set; explicitly assigning the default value can help prevent potential inconsistencies in reaction behavior.
| types::ReactionComponent aerosol_phase_water_component; | |
| types::ReactionComponent aerosol_phase_water_component; | |
| aerosol_phase_water_component.coefficient = 0.0; // Explicitly set default coefficient |
|
I like Kyle's suggestion. The code looks good to me, nothing to mention particularly. I'll review once more when all the tests pass. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds an in‑code option for defining chemical mechanisms directly in Python and updates several reaction component definitions to use a structured ReactionComponent instead of a simple string. Key changes include:
- Adding coefficient checks for reaction components in tests.
- Converting gas_phase_species and aerosol_phase_species from strings to ReactionComponent structures in both tests and parsers.
- Updating Python bindings and examples to reflect renamings and new struct representations.
Reviewed Changes
Copilot reviewed 13 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/v1/test_parse_simpol_phase_transfer.cpp | Added coefficient validations for simpol phase transfer reaction components. |
| test/unit/v1/test_parse_henrys_law.cpp | Updated gas and aerosol phase species checks to use ReactionComponent. |
| test/unit/v1/test_parse_condensed_phase_photolysis.cpp | Adjusted scaling factor field name to match the new field. |
| src/v1/henrys_law_parser.cpp | Changed assignment to construct a ReactionComponent for gas and aerosol phases. |
| src/v1/condensed_phase_photolysis_parser.cpp | Renamed scaling factor field to match updated struct. |
| mechanism_configuration/wrapper.cpp | Updated Pybind11 bindings, including renaming unknown_properties to other_properties and reorganizing the module structure. |
| include/mechanism_configuration/v1/types.hpp | Modified ReactionComponent fields and replaced fields with the new struct format. |
| examples/v1/full_configuration.yaml | Updated example configurations to align with new reaction and arrhenius definitions. |
Files not reviewed (4)
- CMakeLists.txt: Language not supported
- Dockerfile: Language not supported
- examples/v1/full_configuration.json: Language not supported
- mechanism_configuration/CMakeLists.txt: Language not supported
Comments suppressed due to low confidence (2)
python/tests/test_parser.py:1
- The complete removal of test_parser.py may result in reduced coverage of the Python parser functionality. Please ensure that alternative tests are in place to validate the new in‑code mechanism feature.
import pytest
mechanism_configuration/wrapper.cpp:194
- [nitpick] The Python binding has renamed unknown_properties to other_properties. Consider updating the documentation/comments to clarify this naming choice, ensuring consistency for API users.
.def_readwrite("other_properties", &Species::unknown_properties)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #90 +/- ##
=======================================
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:
|
Adds the option to define chemical mechanisms directly in Python, without parsing JSON/YAML files. Also, updates a few minor inconsistencies in existing struct data member names and types.
The python test of the parser now includes validation of each component, and a test of an in-code mechanism equivalent to what is in the JSON/YAML test configuration files.
Let me know if there are ways to improve the Python API for creating the structs. I'm not sure if it's the most "Python" way of doing something like this.