Added method from arrhenius to Arrhenius ep #1097
Conversation
goldmanm
force-pushed
the
added_method_from_Arrhenius_to_ArrheniusEP
branch
from
1fba017 to
78a59ab
Compare
|
Why is it desirable to store Arrhenius kinetics as ArrheniusEP kinetics that are equivalent to the Arrhenius kinetics? These aren't even really valid ArrheniusEP kinetics because they are defined entirely independent of the heat of reaction. |
|
Only reason I can think of is so one can average it with something that is |
rmgpy/kinetics/arrhenius.pyx
Outdated
| @@ -249,7 +249,23 @@ cdef class Arrhenius(KineticsModel): | |||
|
|
|||
| # Set the rate parameter to a cantera Arrhenius object | |||
| ctReaction.rate = self.toCanteraKinetics() | |||
|
|
|||
|
|
|||
| cpdef ArrheniusEP toArrheniusEP(self): | |||
There was a problem hiding this comment.
Can we at least have the option of setting alpha, even if we don't use it currently?
There was a problem hiding this comment.
Yeah. I can set an alpha option, though I am hesitant to add features that may never be used. Other places in the code that convert Arrhenius to ArrheniusEP set alpha equal to 0.
Do you know when/how we would use the extra parameter?
There was a problem hiding this comment.
In order to convert Arrhenius to ArrheniusEP with a nonzero alpha you would need to know the ∆H of the reaction, which presumably is not known when this is called. I think it would probably be not only an unused feature, but an almost-impossible-to-use feature.
There was a problem hiding this comment.
Sorry, I guess it's a little bit more complex than that. Knowing Ea and alpha is sufficient to back out E0 which with alpha defines the EP kinetics.
The reason I think we should have this is that I generally believe that functions should be general to all cases they might be used in and operate in a way that makes sense to the uninitiated. This function sounds like it does something that physically make sense: if you can estimate the alpha and you know the dHrxn and Ea you can generate an EP rule. Instead it only makes rules with E0 = Ea, which makes no physical sense unless alpha really is 0. If someone were looking to convert an Arrhenius object into a real EP rule, this function would look like what they were suppose to use, when in fact it does nothing related to that at all. However, luckily in this case we can relatively easily make it function the physically sensical way if alpha is set and the programmatic way if alpha is left out, satisfying both possibilities.
At least I think this is easy to do?
There was a problem hiding this comment.
Yes you could do it, and it may well have good uses, but you would need to pass it ∆H as well as alpha if alpha ≠ 0. But would you enforce ∆H is always passed to this function, or let it be omitted if alpha is also omitted?
There was a problem hiding this comment.
Sorry, yes you do need dHrxn. If we default it to dHrxn=0.0 we just have to replace Ea with Ea-alpha*dHrxn and it works for both cases.
I agree that it would be an almost-impossible-to-use feature within the RMG context, but I know from experience that RMG users may use our code in ways we didn't intend and it shouldn't take more than a few seconds for us to add this functionality.
There was a problem hiding this comment.
I added a method to allow the alpha to be changed, as some people might want to use it (though in my experience, it is unlikely to occur before the code is widely rewritten). I also added test methods for it as well.
I agree with @rwest that deltaH would need to be passed so there is an exception if only alpha or deltaH is set. If only alpha is passed the ArrheniusEP would be wrong and the user would get incorrect values.
|
I guess a bit of why I wrote this PR. For some reason, my computer and pharos were getting an error when loading the database since it was trying to average Arrhenius with ArrheniusEP values (in KineticsRules.__getAverageKinetics). I found out that there were kinetics from RateRules that were getting through. The RMG master branch was passing on Travis. I went to the master branch on my computer, Instead of manually writing a conversion, I wrote a method in the Arrhenius class to convert it. I am not sure if we should be converting all rate rules from Arrhenius to ArrheniusEP. I couldn't find where rate rule conversions occured before __getAverageKinetics. I don't think it should affect the output significantly. These changes fixed this issue that for some reason only I was seeing. |
|
If only you were seeing the issue, it makes me wonder if/how your database is different? Could lead to some insight. (Can't think what else would cause yours to behave differently) |
|
Great intuition @rwest. Much insight beyond the My database definitely caused the bug. I cherrypicked the most recent commit, which removed a rate rule (see database issue 199), which was causing the bug. Removing this group additivity rate rule, index 5 in r addition multiplebond (large file warning) caused the previously unused rule index 3133, which has Arrhenius kinetics, to be exposed. Averaging with this node caused the error. If you look at the commit that added rule 3133, it mentions this rate should overrule the GAV rule that I deleted but erroneously labeled the wrong rank to them. I guess a question is: Should we allow rate rules to be input as Arrhenius at all? Maybe this PR could fix any that are mistakenly entered as Arrhenius. Another option is to have a unittest that ensures all entries are ArrheniusEP. This also highlights that we have some better kinetics that are not being used currently in RMG. |
There are already locations in the code where Arrhenius is converted into ArrheniusEP. This commit allows for one central location to do the conversion.
To my knowledge, rate rules should be stored as ArrheniusEP. For some reason, RMG was not converting those stored as Arrhenius into ArrheniusEP. This converts them when they enter into the database.
goldmanm
force-pushed
the
added_method_from_Arrhenius_to_ArrheniusEP
branch
from
78a59ab to
d6c9fb7
Compare
There are already locations in the code where Arrhenius is converted into ArrheniusEP. This commit allows for one central location to do the conversion.
I replaced the instances where the conversion currently happens.
I added a new instance when loading rate rules that have Arrhenius objects.
Sidenote: for some reason loading some parts of the database were not working for me (example testGenerateReactions), and this PR fixed it.