Merge model improvements #1649
Merge model improvements #1649
Conversation
rmgpy/tools/merge_models.py
Outdated
| # ensure no species with same name and index | ||
| label_index_dict = {} | ||
| for s in final_model.species: | ||
| if s.label not in label_index_dict.keys(): |
There was a problem hiding this comment.
This can just be if s.label not in label_index_dict. No need to call keys().
There was a problem hiding this comment.
good to know! Just made the code simpler!
| else: | ||
| if s.index in label_index_dict[s.label]: | ||
| # obtained a duplicate | ||
| s.index = max(label_index_dict[s.label]) + 1 |
There was a problem hiding this comment.
Do we want to have unique indices? In other words, do we want to reindex starting from the largest index in the entire model + 1?
There was a problem hiding this comment.
This code isn't guaranteeing unique indexes at all. To do that, we would pretty much have to renumber everything except the first model, which would make this PR much less useful.
From my point of view, if the user starts with an un-indexed mechanism, like aramcomech, the output should also not have indexes.
There was a problem hiding this comment.
Is there a benefit to unique indices that I've overlooked? My main goal for this PR was to not have any CHEMKIN or cantera strings conflict, while keeping those names as similar as possible to the input names.
There was a problem hiding this comment.
To give an example of the functioning in this PR: if model 1 has CH3(15) and model 2 has H2O(15), then neither of their names would be changed, since both the name and index don't match.
I would think for many users, avoiding chemkin or cantera names changing would be preferable to ensuring every species has a unique number.
There was a problem hiding this comment.
Let me know your thoughts on this
There was a problem hiding this comment.
Ah, I forgot that we would also need to check for species which already have the same indices.
I think it would only matter if you were merging RMG models and wanted to load the merged model back into RMG, which would parse the indices from the labels. I'm not sure exactly what effect duplicate indices would have in that scenario though. It might not actually matter.
I'm ok with the behavior you implemented. I think the key desired outcome is that the resulting merged mechanism file is directly simulate-able in Chemkin or Cantera.
rmgpy/rmg/model.py
Outdated
| #for spec in finalModel.species: | ||
| # if spec.label not in ['Ar','N2','Ne','He']: | ||
| # spec.index = speciesIndex + 1 | ||
| # speciesIndex += 1 |
There was a problem hiding this comment.
Since you implemented a different indexing system, I would just remove this section instead of commenting it out.
goldmanm
force-pushed
the
merge_model_improvements
branch
from
0d32c24 to
3e4272a
Compare
Codecov Report
@@ Coverage Diff @@
## master #1649 +/- ##
=========================================
+ Coverage 41.68% 41.89% +0.2%
=========================================
Files 176 176
Lines 29341 29354 +13
Branches 6049 6052 +3
=========================================
+ Hits 12232 12299 +67
+ Misses 16239 16179 -60
- Partials 870 876 +6
Continue to review full report at Codecov.
|
goldmanm
force-pushed
the
merge_model_improvements
branch
2 times, most recently
from
87f2160 to
eb995cf
Compare
Previously merge_models would error if no species or reactions found. This commit prevents the divide by zero error from stopping merge_models.
This commit refactors merge_model to separate out the merging and saving of the model so that it can be effectively unittested.
This commit adds unittests to test merge_model functionality.
This commit replaces a complete reindex for merged models with a smart (as in 'smart speaker') algorithm that only reindexes a species when it is has both an identical label and index with another species. This prevents conflicts and reduces unnecessary reindexing.
goldmanm
force-pushed
the
merge_model_improvements
branch
from
eb995cf to
a4f0320
Compare
Motivation or Problem
merge_models has some unexpected behavior, like throwing divide by zero errors when empty models are given and renaming the species in a chemkin model
Description of Changes
This PR cleans up some of the workings of the merge_models tool
executemethod to call two separate methods that perform different tasks, which allows for better unittestingTesting
Haha. I ran the new unittest. :) I also ran merge model on minimal and superminimal examples.
Reviewer Tips
Run merge models on two of your favorite RMG generated models :)