Skip to content
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

Merged
merged 7 commits into from
Oct 10, 2019
Merged

Dms oxy changes #360

merged 7 commits into from
Oct 10, 2019

Conversation

rgillis8
Copy link
Contributor

@rgillis8 rgillis8 commented Oct 4, 2019

This pull request encompasses several changes to the database related to my work on DMS oxidation.

  1. The largest change is a revamp of the group additivity scheme for oxygenated sulfur molecules.
  2. It also includes an additional reaction library,
  3. Several more training reactions for H Abstraction,
  4. A bugfix for the Intra_R_Add_Endocyclic Family nodes (The top node wasn't sufficiently general)

@rgillis8 rgillis8 self-assigned this Oct 4, 2019
@rgillis8 rgillis8 requested a review from xiaoruiDong October 4, 2019 18:13
Copy link
Contributor

@xiaoruiDong xiaoruiDong left a 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.

  1. 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? )
  2. 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

Comment on lines +43531 to +43549
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"""
""",
)

Copy link
Contributor

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!

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

@rgillis8
Copy link
Contributor Author

rgillis8 commented Oct 4, 2019

  1. These are almost entirely based on my own calculations (generally CBS-QB3, noted in the entries). There are a few literature values in the libraries.

  2. Actually, let me look at these database test errors... I think I may have missed them in my previous sweep. I seem to have done something strange causing the database tests to take forever to run. I'll investigate this further.

Copy link
Contributor

@xiaoruiDong xiaoruiDong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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!
  2. After your investigation, let me know and I will try to run the test on my computer.
    Thanks!

@rgillis8
Copy link
Contributor Author

rgillis8 commented Oct 4, 2019

  1. a. I calculated 300 molecules, added in 150 literature values.
    b. Based on those 450 molecules I designed the trees (not automatic) that I thought balanced specificity with ability to solve.
    c. I automatically generated the matrices needed to fit the molecule thermochemistry to the groups. This involved
    d. I fit and validated the thermo groups. Avg error is something like 0.7 kcal/mol for enthalpy between calculation is fit.
    e. I wrote a new group.py file, automatically writing over entries that were refitted.

  2. Sure.

@rgillis8
Copy link
Contributor Author

rgillis8 commented Oct 7, 2019

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!

@xiaoruiDong xiaoruiDong self-requested a review October 8, 2019 19:17
@rgillis8 rgillis8 requested a review from mjohnson541 October 9, 2019 13:38
@rgillis8
Copy link
Contributor Author

@mjohnson541
Ok. I've looked over the Sulfur case and it seems to be behaving how I expect it to. Let me know if there are any other checks you want me to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants