New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix multi pdep arrhenius calculation #1341
Conversation
Are these two calculation approaches actually the same? I was looking through git history yesterday to understand why we have
Based on those commits, I don't think that this is an appropriate fix for this issue. At the very least, I think we need to look deeper into the difference between the calculation methods. |
Suppose they were different, the point of having a MultiPdepArrhenius is to combine multiple PdepArrhenius rates together so they can be the kinetics for a single reaction object. Why should the rate of a MultiPdepArrhenius object be different from the sum of the rates of the PdepArrhenius objects it contains? I should also clarify that we have a test that checks that a particular MultiPdepArrhenius evaluates to particular values and both versions pass that test. |
Codecov Report
@@ Coverage Diff @@
## master #1341 +/- ##
=========================================
- Coverage 41.31% 41.3% -0.01%
=========================================
Files 178 178
Lines 30348 30351 +3
Branches 6140 6141 +1
=========================================
Hits 12537 12537
- Misses 16948 16950 +2
- Partials 863 864 +1
Continue to review full report at Codecov.
|
After looking at it some more, I'm now convinced that the previous method wasn't doing anything special. The only difference should be the check to ensure identical plists. I think this PR can be merged to address ReactionMechanismGenerator/RMG-website#165. Following this change, there is effectively no difference between @rwest, I know this is from a long time ago, but do you remember any details regarding the reasoning for the current implementation of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The technical changes look good, I agree this should be the solution. Thanks @mjohnson541. If everything else is accepted on everyone, this could be merged.
testing/databaseTest.py
Outdated
This test ensures that every library reaction has evaluable kinetics | ||
""" | ||
for entry in library.entries.values(): | ||
entry.data.getRateCoefficient(1000.0,1.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also add some additional checks here? For example, check that the rate is non-negative and within a reasonable range?
Do we want to proceed with this solution? |
Yes, I think we can proceed with this. Do you want to add additional checks like @cgrambow suggested? |
Yes, we can check that it's nonnegative pretty easily, @cgrambow were you thinking of checking the diffusion and TST limits? |
Yeah, that probably makes the most sense. |
6372753
to
b11138a
Compare
So I just pushed positivity, TST and collision limit tests and we have a lot of failures in existing libraries:
|
I tested the rates at 1000 K 1 bar, the collision and TST limit calculations should be in the commit log. |
b11138a
to
eb3210b
Compare
One of the reactions this reports is: |
testing/databaseTest.py
Outdated
@@ -443,14 +443,14 @@ def kinetics_checkLibraryRatesAreReasonable(self, library): | |||
if k < 0: | |||
boo = True | |||
logging.error('library reaction {0} from library {1}, had a negative rate at 1000 K, 1 bar'.format(rxn,library.label)) | |||
if len(rxn.reactants) == 1: | |||
if len(rxn.reactants) == 1 and rxn.allow_max_rate_violation==False: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test fails with AttributeError: 'rmgpy.reaction.Reaction' object has no attribute 'allow_max_rate_violation'
, I think you should first check hasattr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I forgot to actually set the attribute Reaction.init
2b44210
to
ab2430a
Compare
@mjohnson541, I still get an error: |
5515206
to
9568307
Compare
cf4acda
to
27b0e3e
Compare
Any chance we could separate this from the collision limit check? It would be nice to fix the rate evaluation. |
The collision limit check stuff is almost fixed I think we're just waiting for @jimchu10 to look at the aromatic ones. |
27b0e3e
to
99ec8a6
Compare
…f the PdepArrhenius objects within it before there was a requirement that the pressure lists of each PdepArrhenius objects had to be the same
checks that a 1000 K 1 bar all rates are positive and less than the TST/collision limit
…ax rate requirements
99ec8a6
to
cbc7d4a
Compare
I went ahead and rebased this. Any final considerations before merging? |
Together with the database maxviol branch there shouldn’t be any issues,
might be some issues merging them individually. You could merge the unit
test separately and merge the rest of the RMG-Py branch first then the
database branch and then the unit test. I think that would avoid unit
tests failing.
Best,
Matt
…On Wed, Jul 4, 2018 at 11:49 AM Max Liu ***@***.***> wrote:
I went ahead and rebased this. Any final considerations before merging?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1341 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHacLCDMAAEAX7cSY2AONhhtFDtC42dEks5uDOQKgaJpZM4TKb3h>
.
|
cbc7d4a
to
d01dc79
Compare
This removes the requirement that PdepArrhenius objects in a MultiPdepArrhenius must have the same pressure lists by changing the getRateCoefficient function to evaluate each PdepArrhenius object independently. Also adds a database test that checks that all library reactions can be evaluated.