Add a search filter to merge models #1986
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1986 +/- ##
==========================================
+ Coverage 55.32% 55.33% +0.01%
==========================================
Files 125 125
Lines 37143 37158 +15
==========================================
+ Hits 20550 20563 +13
- Misses 16593 16595 +2 ☔ View full report in Codecov by Sentry. |
|
Once you've done the species comparison you can map names between models and you know what species are only in model 1 and what species are only in model 2. When comparing reactions can't you just compare whether not there's a reaction with the same appropriately mapped reactant and product names? For example for a reaction from model 1 can't you just convert the names to model 2 (if you can't it's unique) and then compare the names with the reactant and product names for each reaction in model 2? |
Absolutely. I'll take another spin at doing the comparison that way. The name comparison is a lot faster than the isomorphism check so that would speed things up a lot. Without the filter and the dictionary key check, it will still be doing this check a lot of times, we'll see what speed looks like. Maybe I can find a way to key a dictionary with a name set for a filtered search. Seems like the molecular weight check is still something that will work well for the species search even if I take it off of the reaction search. This approach does rest on the assumption that the original model will not have species that are isomorphic matches with each other. This is probably an assumption we are comfortable with. The question about how to deal with duplicates remains. Though I think I'm coming back around to the position that we shouldn't add any number of instances of a reaction if there are any versions of it in the original model. |
|
This pull request is being automatically marked as stale because it has not received any interaction in the last 90 days. Please leave a comment if this is still a relevant pull request, otherwise it will automatically be closed in 30 days. |
|
This pull request is being automatically marked as stale because it has not received any interaction in the last 90 days. Please leave a comment if this is still a relevant pull request, otherwise it will automatically be closed in 30 days. |
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:01:24 aromatics Passed Core Comparison ✅Original model has 15 species. aromatics Passed Edge Comparison ✅Original model has 106 species.
Observables 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:02:52 liquid_oxidation Failed Core Comparison ❌Original model has 37 species. Non-identical kinetics! ❌
kinetics: liquid_oxidation Failed Edge Comparison ❌Original model has 202 species. Non-identical kinetics! ❌
kinetics:
Observables 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:47 nitrogen Failed Core Comparison ❌Original model has 41 species. nitrogen Failed Edge Comparison ❌Original model has 132 species.
Observables 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:03:17 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Passed Edge Comparison ✅Original model has 230 species.
Observables 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 ✅Regression test sulfur:Reference: Execution time (DD:HH:MM:SS): 00:00:01:08 sulfur Passed Core Comparison ✅Original model has 27 species. sulfur Failed Edge Comparison ❌Original model has 89 species.
Observables Test Case: SO2 Comparison
The following observables did not match: ❌ Observable species O=S=O varied by more than 0.100 on average between old model SO2(15) and new model SO2(15) in condition 1.
sulfur Failed Observable Testing ❌Regression test superminimal:Reference: Execution time (DD:HH:MM:SS): 00:00:00:43 superminimal Passed Core Comparison ✅Original model has 13 species. superminimal Failed Edge Comparison ❌Original model has 18 species. Non-identical kinetics! ❌
kinetics: Regression test RMS_constantVIdealGasReactor_superminimal:Reference: Execution time (DD:HH:MM:SS): 00:00:03:17 RMS_constantVIdealGasReactor_superminimal Passed Core Comparison ✅Original model has 13 species. RMS_constantVIdealGasReactor_superminimal Passed Edge Comparison ✅Original model has 13 species.
Observables Test Case: RMS_constantVIdealGasReactor_superminimal Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_constantVIdealGasReactor_superminimal Passed Observable Testing ✅Regression test RMS_CSTR_liquid_oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:08:09 RMS_CSTR_liquid_oxidation Failed Core Comparison ❌Original model has 37 species. RMS_CSTR_liquid_oxidation Failed Edge Comparison ❌Original model has 206 species. Non-identical kinetics! ❌
kinetics:
Observables Test Case: RMS_CSTR_liquid_oxidation Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_CSTR_liquid_oxidation Passed Observable Testing ✅beep boop this comment was written by a bot 🤖 |
|
This pull request is being automatically marked as stale because it has not received any interaction in the last 90 days. Please leave a comment if this is still a relevant pull request, otherwise it will automatically be closed in 30 days. |
github-actions
bot
added
stale
|
All parties originally involved are otherwise engaged, and this is a nice to have not a need to have - closing this. |
Motivation or Problem
The mergeModels.py script runs very slowly for large models, taking hours or days for moderately large models. This appears to be driven by the number of isomorphic checks that must be run, with every species from model2 having to be compared against most species in model1 and the same for reactions.
Description of Changes
Added a molecular weight filter for the species search, where species from model2 will only be compared against species in model1 with the same molecular weight. Similarly for reaction comparisons, reactions will only be compared against reactions that have the same sum of molecular weights for their reactants. This is done by placing the species and reactions in search dictionaries keyed by their molecular weights.
After a species (or reaction) from model2 is matched to one from model1, that species is removed from the model1 search dictionary. After a match, no subsequent species in model2 should be expected to match that species again. This is a secondary speed up function, only becoming significant when the model is very large and has many species or reactions with the same weight.
Testing
Passed the unit test for mergeModelsTest.py.
Tested a combination of the outputs from rmg examples ethane-oxidation and ch3no2. Same number of species and reactions in the result for this branch as for the master.
Merged a model for dodecane pyrolysis with itself (438 species, 12510 reactions). No erroneous nonmatches were added. The run time on a laptop was reduced from 35 minutes to 28 seconds. This speed increase was achieved with just the first change. A model large enough to observe benefits from the second change has not been attempted.
Reviewer Tips
The second change would affect how duplicate reactions are handled by the merge function. As of now, the merge function would block any number of reactions in the merging model as long as there was at least 1 identical reaction in the original. This change would allow through a number of reactions in excess of what was in the original (I believe chosen at random). This would be a change for when the merging model has more versions of a reaction than the original, but it's not entirely clear that duplicate reactions are being handled well as it stands now. This has the potential to provide a further speed increase with uncertain implications for the edge case where there are duplicate reactions.
I'm not advocating for this necessarily if this is any kind of regularly encountered situation, but the existing handling of duplicates is somewhat strange so I don't know if this is disqualifying.
Example for A reactions in original merged with B reactions in new model.
Current form:
With Change: