Correct constant-species index mapping in SimpleReactor #2867
Conversation
alongd
changed the title
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in SimpleReactor.get_const_spc_indices() that prevented constant species from being properly identified and treated as constant during simulation. The original implementation had two major issues: it reused the same loop variable spc in nested loops, and it incorrectly compared a string to a Species object (spc.label == spc instead of spc.label == name). As a result, species listed in const_spc_names were never actually marked as constant.
- Fixed variable shadowing in nested loops by using distinct variable names (
namefor const_spc_names,spcfor core_species) - Corrected the comparison logic to properly match species labels against constant species names
- Added early return for
Noneconst_spc_names and moved initialization outside the loop for clarity - Added a
breakstatement to stop searching once a match is found
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| rmgpy/solver/simple.pyx | Fixed the get_const_spc_indices() method by correcting variable naming, comparison logic, and control flow |
| test/rmgpy/solver/simpleTest.py | Added unit test test_get_const_spc_indices() to verify that constant species names are correctly mapped to core species indices |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
A test unrelated to this PR failed: Added another commit to check isclose() for |
|
@JacksonBurns, any ideas why the Conda Build fails here? |
|
CI issue; @jonwzheng has fixed it here: #2869 |
JacksonBurns
force-pushed
the
simp.cnst
branch
from
853030a to
a60f1a3
Compare
JacksonBurns
left a comment
JacksonBurns
left a comment
There was a problem hiding this comment.
This looks reasonable - waiting for the CI to run so we can see if the new tests pass and the regression tests look OK.
Suggested one small change, which I think matters a lot. If get_const_spc_indices is called multiple times, they constant species will be re-appended to the list each time. Double check my understanding here, and that my change actually fixes what I think is wrong.
| if self.const_spc_indices is None: | ||
| self.const_spc_indices = [] | ||
| for name in self.const_spc_names: |
There was a problem hiding this comment.
| if self.const_spc_indices is None: | |
| self.const_spc_indices = [] | |
| for name in self.const_spc_names: | |
| if self.const_spc_indices is None: | |
| self.const_spc_indices = [] | |
| else: | |
| return # this has already been run, results are cached | |
| for name in self.const_spc_names: |
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:00:50 aromatics Passed Core Comparison ✅Original model has 15 species. aromatics Failed Edge Comparison ❌Original model has 106 species. Non-identical thermo! ❌
Identical thermo comments: Non-identical thermo! ❌
thermo: Thermo group additivity estimation: group(Cs-(Cds-Cds)(Cds-Cds)(Cds-Cds)H) + group(Cds-Cds(Cds-Cds)(Cds-Cds)) + group(Cds-CdsCsH) + group(Cds-CdsCsH) + group(Cds-Cds(Cds-Cds)H) + group(Cds-Cds(Cds-Cds)H) + group(Cds-CdsCsH) + group(Cdd-CdsCds) + Estimated bicyclic component: polycyclic(s4_6_6_ane) - ring(Cyclohexane) - ring(Cyclohexane) + ring(124cyclohexatriene) + ring(124cyclohexatriene) Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: DetailsObservables Test Case: Aromatics Comparison✅ All Observables varied by less than 0.500 on average between old model and new model in all conditions! aromatics Passed Observable Testing ✅Regression test liquid_oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:01:57 liquid_oxidation Passed Core Comparison ✅Original model has 37 species. liquid_oxidation Failed Edge Comparison ❌Original model has 214 species. DetailsObservables Test Case: liquid_oxidation Comparison✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! liquid_oxidation Passed Observable Testing ✅Regression test nitrogen:Reference: Execution time (DD:HH:MM:SS): 00:00:01:02 nitrogen Failed Core Comparison ❌Original model has 41 species. nitrogen Failed Edge Comparison ❌Original model has 133 species. DetailsObservables Test Case: NC Comparison✅ All Observables varied by less than 0.200 on average between old model and new model in all conditions! nitrogen Passed Observable Testing ✅Regression test oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:01:48 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Passed Edge Comparison ✅Original model has 230 species. DetailsObservables Test Case: Oxidation Comparison✅ All Observables varied by less than 0.500 on average between old model and new model in all conditions! oxidation Passed Observable Testing ✅Errors occurred during observable testing
WARNING:root:Initial mole fractions do not sum to one; normalizing.
|
Motivation or Problem
SimpleReactor.get_const_spc_indices()was possibly effectively broken:spc.label == spc, i.e. a string to a species object.As a result,
const_spc_indiceswas never populated and species listed inconst_spc_nameswere perhaps not actually treated as constant.Description of Changes
In
SimpleReactor.get_const_spc_indices():const_spc_namesandcore_species.spc.labelinstead ofspcitself.const_spc_indicesonce and append core-species indices when labels match.const_spc_namesisNone.Testing
A small unit test was added.
Reviewer Tips
In a small reactor run with a constant species, check that its amount stays fixed over time.