Add a parser for models #107
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #107 +/- ##
=======================================
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:
|
mattldawson
left a comment
mattldawson
left a comment
There was a problem hiding this comment.
Looks great! I just had a couple minor comments that I realized later were for existing code that was just being moved. So, feel free to ignore them if they seem out of scope for this PR
|
|
||
| if (!ContainsUniqueObjectsByName<types::Species>(all_species)) | ||
| { | ||
| errors.push_back({ ConfigParseStatus::DuplicateSpeciesDetected, "Duplicate species detected." }); |
There was a problem hiding this comment.
Would it be possible to include the name of the duplicated species? It might make it easier to fix in large mechanisms. And maybe include the location in the configuration file similar to errors below? (if that's possible)
There was a problem hiding this comment.
That's a great suggestion. I created a separate issue #111 to address this, since the current one is already quite large.
|
|
||
| if (RequiresUnknownSpecies(species, existing_species)) | ||
| { | ||
| errors.push_back({ ConfigParseStatus::PhaseRequiresUnknownSpecies, "Phase requires unknown species." }); |
There was a problem hiding this comment.
I was a little confused by what this meant at first. What about something like "Phase includes unknown species: 'foo'" and including the name of the species?
There was a problem hiding this comment.
I agree with you. I created a new issue #112 to address that.
|
|
||
| if (!ContainsUniqueObjectsByName<types::Phase>(all_phases)) | ||
| { | ||
| errors.push_back({ ConfigParseStatus::DuplicatePhasesDetected, "Duplicate phases detected." }); |
There was a problem hiding this comment.
maybe include the phase name in the error message?
There was a problem hiding this comment.
That's a great suggestion. I created a separate issue #111 to address this, since the current one is already quite large.
| { | ||
| "name": "gas", | ||
| "type": "GAS_PHASE", | ||
| "phase": "gas" | ||
| }, |
There was a problem hiding this comment.
I don't really understand the point of this. Do we really need this @mattldawson?
K20shores
left a comment
K20shores
left a comment
There was a problem hiding this comment.
I see nothing wrong, I just don't understand that purpose of the gas model, but that shouldn't hold up a merge
This PR
Note:
While this PR is large, most of part is restructuring the reactions code to accommodate the new model configuration supports. No changes have been made to the existing reaction code.