RMG-Cat!!! #1537
Merged
RMG-Cat!!! #1537
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
Closed
|
I think the current problem is cfgoldsmith#42 which is still a bit mysterious. |
rwest
force-pushed
the
catmerge
branch
7 times, most recently
from
030761c to
7dec096
Compare
Codecov Report
@@ Coverage Diff @@
## master #1537 +/- ##
=========================================
- Coverage 41.65% 41.6% -0.06%
=========================================
Files 166 176 +10
Lines 28452 29138 +686
Branches 5855 5991 +136
=========================================
+ Hits 11853 12122 +269
- Misses 15777 16176 +399
- Partials 822 840 +18
Continue to review full report at Codecov.
|
rwest
force-pushed
the
catmerge
branch
3 times, most recently
from
5499040 to
4ce93a8
Compare
rwest
force-pushed
the
catmerge
branch
3 times, most recently
from
28d9f7f to
4671bb1
Compare
|
Can't seem to get the I think the code is ready for a first review; documentation will be coming shortly (there's a PR at cfgoldsmith#56 ) |
amarkpayne
reviewed
Apr 25, 2019
rmgpy/reaction.py
Outdated
| if r.containsSurfaceSite(): | ||
| rateCoefficient /= surfaceSiteDensity | ||
| else: | ||
| adsorbate = r |
There was a problem hiding this comment.
I am adding a check here to make sure to throw an error if there is more than one adsorbate
mliu49
reviewed
Apr 25, 2019
| assert not species.isSurfaceSite(), "Can't estimate thermo of vacant site. Should be in library (and should be 0)" | ||
|
|
||
| logging.debug(("Trying to generate thermo for surface species" | ||
| " with these {} resonance isomer(s):").format(len(species.molecule))) |
There was a problem hiding this comment.
This doesn't really seem like a useful debug message. Was the intention to log the adjlists as well?
| raise | ||
|
|
||
| # (groupAdditivity=True means it appends the comments) | ||
| addThermoData(thermo, adsorptionThermo, groupAdditivity=True) |
There was a problem hiding this comment.
Was there a reason for adding the adsorption correction separately instead of providing thermo directly to __addGroupThermoData?
X is a surface site. Xv is a vacant surface site. Xo is an occupied surface site. They have 0 covalent radius. They are represented by Ni in RDKit (and hence SMILES strings) Add surface site 'X' to allElements list in atomtype.py Xo atomtype can form bonds (making Xo) A vdW adsorbate includes an Xo site with no bonds, which could then become bonded via a reaction recipe, meaning the Xo atom type needs to have a valid formBond setting, which is also Xo. Surface site X group atom by default has 0 lone pairs
Just asks the first molecule.
Replaces isSurfaceSpecies()
This is like a RateCoefficient but with more flexible units. To allow for: A + * -> A* A* -> B* A* + B* -> C* 2A + * + * -> A* + A*
…d reaction families to RMG-Cat documentation.
This includes a description of adding binding energies in the input file and a linear scaling relations section. Added references. Added some minor fixes/clarifications.
A list of the current surface reaction libraries. More about adsorption corrections.
I think perhaps a sphinx plugin to MS VS Code created this. Usually things end up in the build folder, not source/_build. But if they do end up here, lets ignore them.
This should allow linear scaling relations to work for N atoms.
…lies Add termolecular and surface termolecular families to testing database
This now stores the surfaceSiteDensity, once per RMG run instead of once per reactor. It's also where you specify the binding energies. The surfaceSiteDensity is now preserved and output in the CHEMKIN files. Closes cfgoldsmith#68
Mostly just the example.
Atom: isSilicon, isNitrogen, isFluorine, isClorine, isIodine, isSulfur, isNOS, isSurfaceSite Molecule: tests for isSurfaceSite
suggestion by @mliu49 I left a type check in there, which may just slow things down. Worth it for the extra safety???
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.
Here is RMG for Heterogeneous Catalysis.
This replaces #1536 which came from my own fork (something that can't be changed once a PR is open).
I'm hopefully going to continue trying to tidy up this pull request via some rebasing to make it easier to review in chunks, but however this is sliced up in terms of commits, I think this PR contains all the actual changes that will be included in merging RMG-Cat, so if anyone brave wants to start reviewing, here at least is something we can start to discuss...
(Oh, it needs the
catmergeversion of the RMG-database too, which should be taken care of for Travis with the temporary commit at the end)