Adding in coverage dependent effects to surface kinetics #2090
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Is able to currently compile, and rmg test errors are as follows: |
|
Current errors: |
|
latest errors.. seems like the attribute is there now but is a nonetype: |
|
Potential issues: |
|
All rmg tests pass now |
|
all rmg tests and database tests pass now!!!!! |
Codecov Report
@@ Coverage Diff @@
## master #2090 +/- ##
==========================================
+ Coverage 47.84% 47.90% +0.05%
==========================================
Files 102 102
Lines 27089 27116 +27
Branches 6948 6957 +9
==========================================
+ Hits 12962 12989 +27
- Misses 12734 12737 +3
+ Partials 1393 1390 -3
Continue to review full report at Codecov.
|
|
fixing up repr current errors: |
|
All new and old rmg tests pass and all new and old database tests pass. Only thing left is cantera export I think |
mazeau
force-pushed
the
cde
branch
8 times, most recently
from
d0df754
to
679a5e8
Compare
rwest
requested changes
Mar 24, 2021
davidfarinajr
requested changes
Mar 24, 2021
There was a problem hiding this comment.
This is looking good @mazeau . I think we should add more type and value checks when setting the cov dep params for kinetics. We should also figure out how to represent species in cov dep dicts, and determine the most efficient way to find those species and apply cov dep correction in the solver
I think we likely need to use species index. Although looking at collision efficiencies, which is somewhat analogous, it uses |
Yes, we should be using species index in the residual function since this should be faster than SMILES lookups. However, we need a way to match species in the model to the species in each kinetics cov dep dict when we initialize the simulation. I think this is what those isomorphism checks are for. We could do that if the cov dep dict keys were RMG mols instead of SMILES. This may take longer then matching SMILES, but it is less error-prone and it would only be run once at the start of simulation. Using RMG mols also solves the issue of H-Pt and Pt having the same SMILES |
This defaults to False if not specified as true. The fact that this wasn't noticed, tells us that the test isn't really testing that we do evaluate coverage-dependence correctly.
Should be (a, m, E).
also, turning the forgotten 'J/mol' into Eunits !
If we're even going to bother writing this code we really ought to test it
Dictionaries are order-preserving, and humans' brains are, so we'll have fewer bugs if we're consistent with ourselves and other software's conventions. Chemkin and Cantera use (a, m, E) ordering. Now we do, throughout.
If there's a STICK line before a COV line we can cope.
The E should be in the same units as Ea, which is kcal/mol. That's 4184 times smaller.
do not write if the coverage dependence dictionary is empty
…ence is turned off" This reverts commit 7841f24. Had to manually tweak a few things to resolve conflicts. The point is to remove the extra code from the chemkin-writing methods. If we turn off coverage, the parameters should not be in the model. If the parameters are in the model, they should be written by chemkin.
- An empty dictionary shouldn't be printed (even though it's not 'None') - We need sticking coefficients cov parameters to be in the right units too! (we previously fixed from J/mol to kcal/mol in one place, but not both)
Protects us from bugs where we fix things in one place and not the other.
…endence. This better describes what it does. Also expanded docstring. Also removed it from INSIDE the apply_kinetics_to_reaction method and removed if from and elif block, so that it is duplicated in less code (but still called in the same way, I think)
I'm still not very sure about the `coverage_dependence=False` settings, and what should be turned off by it.
Putting dictionaries as a default argument is a famous gotcha https://docs.python-guide.org/writing/gotchas/ I wondered if it's being done on purpose for some efficiency reason, to share memory, but decided no. a risk of error, and is not needed anyway as we never initialize the object with those parameters anyway.
I'm hoping to speed it up later.
They look the same :-/
At 600 K the eqm coverage is much higher, so you get a more noticeable coverage dependence. By plotting coverage instead of concentration it's easier to see what's happening.
Add a bunch of redundant species and reactions, to slow it down a bit, for benchmarking purposes. Simulation took 3.550e+00 seconds in rmgpy.solver.surfaceTest.SurfaceReactorCheck.test_solve_h2_coverage_dependence
Main goal was to optimize everything happening inside the loops in the "residual" function (which gets called a lot). Simulation took 4.541e-01 seconds in rmgpy.solver.surfaceTest.SurfaceReactorCheck.test_solve_h2_coverage_dependence (previous run was 3.550e+00, so we're about 8x faster)
These are helpful for manual inspection and helped me confirm it's working, but they're not needed for the automated test suite or continuous integration.
changed CH3 coverage unittest to run an additional simulation without coverage dependence. the solution arrays from each are compared.
added instructions on input file construction and updating the database.
rwest
added a commit
to ReactionMechanismGenerator/RMG-database
that referenced
this pull request
Jul 23, 2021
…ndence Coverage dependence of kinetics. Goes with RMG-Py Pull Request ReactionMechanismGenerator/RMG-Py#2090
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation or Problem
Coverage dependent effects are found in many surface mechanisms, and we have been unable to use them. These effects can become significant at high temperatures and pressures. Additionally, some models would have too much O2 adsorbing to the surface too quickly, blocking anything else from happening.
Description of Changes
A new kinetics attribute called
coverage_dependencewas added that is a dictionary of dictionaries that holds the Species the parameters correspond to and the coverage dependent parameters.Testing
Unit tests to make sure RMG can read in kinetics entries with coverage dependent parameters, write chemkin and cantera output files with the coverage parameters, and use the parameters correctly. We've built a few models with it, and it seems to be working.
Note: dual PR with database.
Goes with ReactionMechanismGenerator/RMG-database#462
The commits that add this to the CI testing should be removed from both PRs before merging.