-
Notifications
You must be signed in to change notification settings - Fork 137
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
Dms oxy changes #360
Dms oxy changes #360
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.
Ryan, thanks for the PR. The codes and syntaxes look nice. Here are a few more commnets for me to better understand this PR.
- Can you provide a reference or briefly tell me about how you make these updates. (what is the workflow, what's the source of data, and any performance-enhancing or other change you observed? )
- I notice that some failures in the test refer to the group additivity for sulfur species. they look similar:
In group radical, a sample molecule made from node S2J-(Cs-Cb) returns node S2J-Cs when descending the tree.
Do you think it is that an expected behavior because of the group additivity database is updated? @mliu49 @mjohnson541
entry( | ||
index = 1400, | ||
label = "Sc", | ||
group = | ||
""" | ||
1 * S ux px c+1 | ||
""", | ||
thermo = ThermoData( | ||
Tdata = ([300,400,500,600,800,1000,1500],'K'), | ||
Cpdata = ([5,5,5,5,5,5,5],'cal/(mol*K)','+|-',[1,1,1,1,1,1,1]), | ||
H298 = (200,'kcal/mol','+|-',1), | ||
S298 = (20,'cal/(mol*K)','+|-',1), | ||
), | ||
shortDesc = u"""Knocks out charged thermo""", | ||
longDesc = | ||
u""" | ||
""", | ||
) | ||
|
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.
Ryan, I am sorry that I am confused. In the message, you say when setting as shown, we can avoid estimation by group additivity. Can you explain how this works? I check the _add_group_thermo_data() and compute_group_additivity_thermo() in rmgpy/data/thermo.py but cannot understand how it is avoided. Thanks!
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.
So the idea is that when there are multiple resonance forms the lowest enthalpy form is selected. Charged species do not have GAV values and thus often default to the top nodes. If the top node over-estimate the stability than an error arises because they mistakenly use GAV values fit to charged species groups to estimate the thermo of molecule.
This gives a high thermo value for the most common charged sulfur species, pushing rmg to use a non-charged resonance form when using GAV to estimate thermo.
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.
Okay, I get it. Since RMG is not dealing with charged species, I think it is okay to use this strategy at the current stage.
And Can you please rephrase the commit message? It may be because of my poor English but I just find it a little misleading. I think we do not avoid group additivity estimation for charged sulfur species, we are just avoid using thermo from the charged molecules (or resonances) as the thermo for the species. Is that correct?
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.
Sure
|
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.
- Great. And can you provide more details about your group additivity update workflow? Since the modification involves thousands of lines, I just want to understand what you did and make sure you did everything systematically!
- After your investigation, let me know and I will try to run the test on my computer.
Thanks!
|
4ddf0ab
to
77097a7
Compare
77097a7
to
91f33be
Compare
…V estimated thermo of charged resonance forms as the representative thermo of sulfur-containing molecules.
…sufficiently general
91f33be
to
6521cc6
Compare
Ok. 3rd times a charm. This version passes the database tests and I rounded the entries so its a bit less obnoxious. I also amended one of the commit messages per Xiaorui's preferences. Let me know how this looks/if there is anything else I should amend. I am eager to get it squished in! |
@mjohnson541 |
This pull request encompasses several changes to the database related to my work on DMS oxidation.