-
Notifications
You must be signed in to change notification settings - Fork 144
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
Hydrazine pyrolysis #296
Hydrazine pyrolysis #296
Conversation
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.
Looking good.
I only reviewed the format and made a couple of comments, I'll take a look at the rates soon.
""", | ||
) | ||
|
||
#entry( |
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.
Let's remove these commented entries from this PR. We also have a reasonable rate for NH2 + N2H3 <=> H2NN(S) + NH3
from D&B, so we might leave these out.
input/thermo/libraries/primaryNS.py
Outdated
@@ -1665,8 +1665,8 @@ | |||
molecule = | |||
""" | |||
1 N u0 p1 c0 {2,S} {4,S} {5,S} | |||
2 N u0 p1 c0 {1,S} {3,S} {6,S} | |||
3 N u0 p2 c0 {2,S} | |||
2 N u0 p0 c+1 {1,S} {3,D} {6,S} |
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.
Could you also change the SMILES in the description below?
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.
Thanks. I corrected it.
I replaced the rate constant of NH2+N2H3=H2NN(S)+NH3 with that proposed by DeanBozz. Also, I removed NH2+N2H3=H2NN(T)+NH3. This reaction with the same rate constant of DeanBozz will be added by RMG as a rate rule. |
da47f9b
to
8215285
Compare
The branch was updated with the following changes:
ReactionMechanismGenerator/RMG-Py#1496 should be merged before this PR |
8215285
to
ce209cc
Compare
2485ddd
to
405ab1b
Compare
c3dd0e4
to
71d4b2a
Compare
I think this PR is now ready for review. |
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.
I have some general comments, but other than that this looks fine.
entry( | ||
index = 0, | ||
label = "NNHNH2", | ||
group = "OR{non_charged, charged}", |
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.
Is it possible to not use an OR?
shortDesc = u"""""", | ||
longDesc = | ||
u""" | ||
Geometry could not converge at CBS-QB3 |
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.
I trust you guys have considered the implications of these forbidden groups, but it's worth noting that an inability to converge a proper geometry doesn't mean a structure couldn't be an intermediate in a concerted reaction RMG takes multiple steps to do.
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.
I haven't thought about that. Does it hold even if I define a forbidden Species and not a Group?
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.
Forbidden species would be less risky because they only apply to that one species. The issue isn't that common, but it is worth considering as forbidding a pathway that's real could be a huge problem for someone later on.
60ba3a6
to
92f8441
Compare
@mjohnson541, I updated this branch. |
So lets say the elementary steps are A => B => C but B doesn't have a stable geometry. Technically the real reaction becomes A => C, but RMG will always find A => B => C instead. This means that forbidding B will prevent the real reaction A => C from being found in RMG. I'm not suggesting this is the case for your forbidden species, but it's worth thinking about. |
Thanks! Definitely something to look out for. I went over the forbidden species/groups list again, I'm relatively comfortable with it. |
988dbfd
to
7b465ac
Compare
@mjohnson541, I rebased and squashed. |
In NOx2018 and NitrogenDean&Bozzelli this reaction (D&B k31f1) is only the direct route of a proccess that also involves a P-dep route
Data taken from the same source Also corrected units of two training reactions
Added elementary_high_p flags and updated degeneracies
to differentiate it from the less stable form H2NN(T)
Also fixed some labeling
Forbidding cases such as: R + C2H3 = RH + [CH]=[CH] (mul) R + NNH = RH + [N]=[N] (mul) R + [NH][NH] <=> RH + [N][NH] (birad)
@mjohnson541, I rebased and it should now be good to go. Let me know if you have further comments |
@alongd can you take a look at the RMG-tests failures on this branch? |
original A factor in the library was wrong, probably a typo
Currently contains molecules related to hydrazine decomposition calculated using the 1DMin code for N2 bath gas.
also for the reverse T.L. Nguyen, J.F. Staton, IJCK 2019, doi: 10.1002/kin.21255
T.L. Nguyen, J.F. Staton, IJCK 2019, doi: 10.1002/kin.21255
I deleted the Travis build cache and restarted the push job, which should re-trigger RMG-tests. Let's see what happens. |
@@ -56,6 +56,8 @@ | |||
'6_membered_central_C-C_shift', | |||
'Intra_R_Add_Exo_scission', | |||
'1,2_shiftC', | |||
'1,2_NH3_elimination', | |||
'1,3_NH3_elimination', |
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.
Do you think this would be a good opportunity to create a separate "nitrogen" set of families? Or do you think they should be included in the default set?
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.
@mliu49, we don't have that many (I only see intra_NO2_ONO_conversion
in addition to the ones added here), I think it's OK to have just one recommended family list to avoid adding another parameter the user has to remember to add in the input file. It could be OK to automatically check for reactive nitrogen in the input first, then let RMG decide which families to use, but it sounds a bit like over-engineering
I figured out why RMG-tests is failing, and it's an extremely stupefying reason... Basically, RMG-tests is set up so that the RMG-Py/database branch to be tested is encoded in the name of the RMG-tests branch which is created and then parsed by RMG-tests. For example, if we're testing the You can probably see where this is going... This branch is called The easiest solution might be to create a new PR under a different branch name (without any hyphens). Ideally, we would update RMG-tests to be more robust, but updating the bash script there would take some effort. |
amazing... |
To avoid mistakes, I'm doing the PR by two steps. This PR added new reactions to primaryNitrogenLibrary but did't change the rate constants of other reactions.
The reactions of NH2+N2H3 were commented out to carry out make test-database.