Reaction Mechanism Simulator YAML File Generation#1629
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1629 +/- ##
==========================================
- Coverage 41.47% 41.37% -0.11%
==========================================
Files 176 177 +1
Lines 29314 29455 +141
Branches 6035 6059 +24
==========================================
+ Hits 12158 12186 +28
- Misses 16303 16417 +114
+ Partials 853 852 -1
Continue to review full report at Codecov.
|
rmgpy/yml.py
Outdated
| from rmgpy.kinetics.arrhenius import * | ||
| from rmgpy.kinetics.falloff import * | ||
| from rmgpy.kinetics.chebyshev import * | ||
| from rmgpy.data.solvation import * |
There was a problem hiding this comment.
Can you replace all of the * imports with explicit ones?
rmgpy/yml.py
Outdated
| from rmgpy.data.solvation import * | ||
| from rmgpy.util import makeOutputSubdirectory | ||
|
|
||
| def convertchemkin2yml(chemkinpath,spcdictpath=None,output="chem.rms"): |
There was a problem hiding this comment.
Can you add spaces to after all commas in this file, in accordance with PEP-8?
rmgpy/yml.py
Outdated
| return D | ||
|
|
||
| def getradicals(spc): | ||
| if spc.molecule[0].toSMILES() == "[O][O]": |
There was a problem hiding this comment.
What is the purpose of this function? Can you not use Molecule.getRadicalCount?
Also, why does oxygen need to be special cased? Could you add a comment explaining?
There was a problem hiding this comment.
I think this is faster since the multiplicities are known.
This is typically used for analyzing radical production in RMS and it's very useful to treat O2 as a stable molecule when you're looking at bulk radical production/loss
rmgpy/yml.py
Outdated
| elif obj is None: | ||
| return None | ||
| else: | ||
| raise ValueError |
There was a problem hiding this comment.
Add a message to this error?
rmgpy/yml.py
Outdated
| D["kinetics"] = obj2dict(obj.kinetics,spcs) | ||
| D["type"] = "ElementaryReaction" | ||
| D["radicalchange"] = sum([getradicals(x) for x in obj.products]) - sum([getradicals(x) for x in obj.reactants]) | ||
| elif isinstance(obj,Arrhenius): |
There was a problem hiding this comment.
Is support for the surface kinetics types necessary? I assume not now, since RMG doesn't have surface reactors?
There was a problem hiding this comment.
I've added them in, RMS can read the catalysis output files properly yet, but it will be able to in the future.
amarkpayne
left a comment
amarkpayne
left a comment
There was a problem hiding this comment.
I have a few quick comments that need addressing, but other than that this looks just about ready to go. The rmgpy/yml.py file was missing a header though, so I have pushed a commit with this fix along with some other small style fixes.
|
|
||
| def getMechDict(spcs, rxns, solvent='solvent', solventData=None): | ||
| D = dict() | ||
| D["Units"] = dict() |
There was a problem hiding this comment.
Does ["Units"] ever get updated anywhere else in the code? Does the YAML format for RMS/Cantera assume that an empty dictionary for units defaults to something?
There was a problem hiding this comment.
It defaults to SI units for everything, unless units are specified for individual floats
rmgpy/yml.py
Outdated
| def getMechDict(spcs, rxns, solvent='solvent', solventData=None): | ||
| D = dict() | ||
| D["Units"] = dict() | ||
| D["Reactions"] = [] |
There was a problem hiding this comment.
This line is not needed right? Further down you have D["Reactions"] = [obj2dict(x, spcs) for x in rxns], which will overwrite this line and also return an empty list if there are no reactions.
rmgpy/yml.py
Outdated
| D["type"] = "Species" | ||
| D["smiles"] = obj.molecule[0].toSMILES() | ||
| D["thermo"] = obj2dict(obj.thermo, spcs) | ||
| if D["smiles"] == "[O][O]": |
There was a problem hiding this comment.
You defined the getradicals function above, so use this to get rid of the if-else statement here.
|
Should be ready to merge now |
|
I agree, feel free to merge this as soon as the tests pass (assuming they do). |
3 participants
Adds generation of .rms YAML files for use in ReactionMechanismSimulator.