Dms oxy changes #1751

rgillis8 · 2019-10-04T17:48:24Z

This short pull request does 3 things, all related to my work on sulfur oxidation systems.

It allows Group Additivity Estimation for sulfur species with hypervalence up to 12 (the previous max was 10, which caused it to be very awkward when GAV was predicting species like CS(=O)(=O)C or the like.
When estimating species thermochemistry by GAV, RMG will now prioritize non-charged resonance forms. If there are multiple non-charged resonance forms then the form with the lowest enthalpy of formation will be selected as the representative thermochemistry. This solves problems where RMG would overestimate the stability of molecules that have charged resonance forms because when GAV is used to estimate the thermo of charged molecules it generally defaults to the top nodes. A test of this is also included to make sure that in the case of multiple non-charged resonance forms, the lower enthalpy form is selected as representative.
It allows simple gas phase reactors to also have constant species (similar to how liquid reactors can have constant species).

Motivation or Problem

A clear and concise description of what what you're trying to fix or improve. Please reference any issues that this addresses.

Description of Changes

A clear and concise description of what what you've changed or added.

Testing

A clear and concise description of testing that you've done or plan to do.

Reviewer Tips

Suggestions for verifying that this PR works or other notes for the reviewer.

rgillis8 · 2019-10-04T20:17:32Z

I would add that it seems to have failed the Travis testing which is strange because it passes locally. Specifically it seems to time out on the the database test, but it is not clear to me why/it doesn't do this on my local test. Maybe Xiaorui could try this and see if it works for him?

mliu49 · 2019-10-04T21:32:59Z

rmgpy/data/thermo.py

+        m = 0        
+        #Pushes all charged species to the back of the list, as a result the lowest enthalpy non-charged resonance form is used for GAV estimation
+        for i in range(len(species.molecule)):
+            if species.molecule[i-m].has_Charge() is True:
+                species.molecule.append(species.molecule[i-m])
+                thermo.append(thermo[i-m])
+                del species.molecule[i-m]
+                del thermo[i-m]
+                m += 1


I would suggest updating the prioritize_thermo method instead, similar to what I did in 85235dd. You can add has_charge as another value in the sorting algorithm.

xiaoruiDong

Thanks, Ryan. It looks good, I just have a few comments about formatting, since we want to keep the PEP-8 formatting in RMG-Py. Thanks.

xiaoruiDong · 2019-10-04T21:25:44Z

rmgpy/molecule/molecule.py

@@ -2066,6 +2066,12 @@ def has_lone_pairs(self):
            if atom.lone_pairs > 0:
                return True
        return False
+
+    def has_Charge(self):


a little comment: has_charge instead of has_Charge.

xiaoruiDong · 2019-10-04T21:29:46Z

rmgpy/rmg/input.py

+            if constantSpecie not in species_dict:
+                raise InputError('Species {0} not found in the input file'.format(constantSpecie))
+
+    if not isinstance(T,list):


Minor: add a space toT,list => T, list

xiaoruiDong · 2019-10-04T21:34:47Z

rmgpy/rmg/input.py

+    #Check the constant species exist
+    if constantSpecies is not None:
+        logging.debug('  Generation with constant species:')
+        for constantSpecie in constantSpecies:


"specie" looks a little bit weird. Do you mind changing it to something else? Maybe like const_spc?

xiaoruiDong · 2019-10-04T21:35:46Z

rmgpy/solver/simple.pyx

@@ -125,6 +127,10 @@ cdef class SimpleReactor(ReactionSystem):

        self.initial_mole_fractions = initial_mole_fractions

+        #Constant Species Properties
+        self.const_spc_indices=None


space before and after =.

xiaoruiDong · 2019-10-04T21:36:25Z

rmgpy/solver/simple.pyx

+        "Allow to identify constant Species position in solver"
+        for spc in self.const_spc_names:
+            if self.const_spc_indices is None: #initialize once the list if constant SPC declared
+                self.const_spc_indices=[]


space before and after =

rgillis8 · 2019-10-07T12:14:14Z

OK this new version responds to each of your concerns, both stylistic and structural. Let me know if you see anything else that is off.

Again, the branches pass all tests locally but the change to allow valence 12 species seems to slow down test-database significantly to the point that it times out. I don't know why having a few more resonance forms to grab thermo for when you are loading libraries would cause such a slowdown, but I can't think of why else loading the libraries would be so slow with the new changes.

mliu49 · 2019-10-07T12:35:33Z

@alongd might have the most insight regarding the effects of allowing valence 12 sulfur species. I think you could try loading the library in question manually to see if you can narrow down the exact species which is taking so much time. It could be either inherent difficulties in generating resonance structures or a bug.

rgillis8 · 2019-10-07T17:30:44Z

So after some investigation I've found that the problem is that the number of resonance forms in species like cyclic S6, S7, and S8 explodes when you allow valence 12 states because it creates a ton of resonance forms with S#S... In this case these are obvious imaginary, but I think that sulfur doesn't form a triple bond with sulfur ever (why would it when it could be a single bond with lone pairs). I think we should amend resonance generation to prevent specifically S#S from lone pairs. @alongd do you have thoughts or strong opinions on this?

alongd · 2019-10-08T00:20:17Z

I'm unaware of cases where S#S is important/representative. I'm OK with excluding it.
We could also just exclude it from ring structures, or just exclude it if the number of resonance structures increases beyond a certain threshold.

rgillis8 · 2019-10-08T15:02:46Z

Ok. I've added another commit that stops the formation of S#S resonance structures from lone pair/multiple bond resonance transitions. This solved the timing issue with large sulfur chains and prevents the timing out observed in the testing. I think these are all ready to merge, but I am happy to change anything else that you see that needs amendment.

codecov · 2019-10-08T16:05:19Z

Codecov Report

Merging #1751 into master will decrease coverage by <.01%.
The diff coverage is 33.33%.

@@            Coverage Diff            @@
##           master   #1751      +/-   ##
=========================================
- Coverage   32.61%   32.6%   -0.01%     
=========================================
  Files          87      87              
  Lines       26103   26115      +12     
  Branches     6869    6875       +6     
=========================================
+ Hits         8514    8516       +2     
+ Misses      16631   16629       -2     
- Partials      958     970      +12

Impacted Files	Coverage Δ
rmgpy/molecule/pathfinder.py	`0% <0%> (ø)`	⬆️
rmgpy/molecule/molecule.py	`0% <0%> (ø)`	⬆️
rmgpy/molecule/filtration.py	`88.33% <100%> (ø)`	⬆️
rmgpy/tools/simulate.py	`79.24% <100%> (+1.88%)`	⬆️
rmgpy/data/thermo.py	`61.79% <100%> (+0.03%)`	⬆️
rmgpy/rmg/input.py	`34% <27.27%> (-0.35%)`	⬇️
rmgpy/data/statmech.py	`42.2% <0%> (ø)`	⬆️
rmgpy/rmg/pdep.py	`12.21% <0%> (ø)`	⬆️
rmgpy/reaction.py	`0% <0%> (ø)`	⬆️
rmgpy/data/kinetics/family.py	`48.28% <0%> (ø)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29e111d...f61752e. Read the comment docs.

xiaoruiDong · 2019-10-08T19:14:58Z

Thanks! I went through the code as well as the test (I also did the tests locally). They look good to me.

lily90502 · 2019-10-08T18:52:14Z

rmgpy/solver/simple.pyx

+        self.const_spc_indices = None
+        self.const_spc_names = const_spc_names #store index of constant species 


I think it's good. Just a minor modification on the commented description.
Might be " self.const_spc_indices = None #store index of constant species"?

mliu49

I made some minor comments, mostly regarding code style.

It would also be nice to add a unit test confirming the resonance changes work as expected.

mliu49 · 2019-10-08T20:04:08Z

rmgpy/molecule/pathfinder.py

+        #if both atom1 and atom2 are sulfur then don't do this type of resonance
+        if not atom1.is_sulfur() or not atom2.is_sulfur():
+           # Bond must be capable of gaining an order
+           if bond12.is_single() or bond12.is_double():
+               for atom3, bond23 in atom2.edges.items():
+                   # Bond must be capable of losing an order without breaking, atom3 must be able to gain a lone pair
+                   if atom1 is not atom3 and (bond23.is_double() or bond23.is_triple()) \
+                           and (atom3.is_carbon() or is_atom_able_to_gain_lone_pair(atom3)):
+                       paths.append([atom1, atom2, atom3, bond12, bond23])


I think you can combine the top two if statements:

if (not atom1.is_sulfur() or not atom2.is_sulfur()) and (bond12.is_single() or bond12.is_double()):

Also line 297 is indented by 3 spaces instead of 4.

mliu49 · 2019-10-08T20:05:04Z

rmgpy/molecule/pathfinder.py

@@ -382,7 +384,7 @@ def find_adj_lone_pair_multiple_bond_delocalization_paths(atom1):
            # Find paths in the direction <increasing> the bond order,
            # atom1 must posses at least one lone pair to loose it
            if ((bond12.is_single() or bond12.is_double())
-                    and is_atom_able_to_lose_lone_pair(atom1)):
+                    and is_atom_able_to_lose_lone_pair(atom1)) and not (atom1.is_sulfur() and atom2.is_sulfur() and bond12.is_double()): #the final clause of this prevents S#S from forming by this resonance pathway


I think this line might be too long.

mliu49 · 2019-10-08T20:05:54Z

rmgpy/solver/simple.pyx

@@ -163,6 +169,15 @@ cdef class SimpleReactor(ReactionSystem):
                    conditions[species_dict[label]] = value
        self.sens_conditions = conditions

+    def get_const_spc_indices (self, coreSpecies):
+        "Allow to identify constant Species position in solver"


Docstrings should be enclosed in triple quotes """docstring"""

mliu49 · 2019-10-08T20:06:34Z

rmgpy/solver/simple.pyx

@@ -163,6 +169,15 @@ cdef class SimpleReactor(ReactionSystem):
                    conditions[species_dict[label]] = value
        self.sens_conditions = conditions

+    def get_const_spc_indices (self, coreSpecies):


Use underscore naming for variables: core_species

mliu49 · 2019-10-08T20:07:07Z

rmgpy/solver/simple.pyx

+        for spc in self.const_spc_names:
+            if self.const_spc_indices is None: #initialize once the list if constant SPC declared
+                self.const_spc_indices = []
+            for iter in coreSpecies: #Need to identify the species object corresponding to the the string written in the input file


iter is a reserved word. Perhaps change to spc or core_spc?

mliu49 · 2019-10-08T20:07:31Z

rmgpy/solver/simple.pyx

+                self.const_spc_indices = []
+            for iter in coreSpecies: #Need to identify the species object corresponding to the the string written in the input file
+                if iter.label == spc:
+                    self.const_spc_indices.append(coreSpecies.index(iter))#get 


The #get comment doesn't seem to make sense.

mliu49 · 2019-10-08T20:08:03Z

rmgpy/solver/simple.pyx

@@ -504,6 +519,10 @@ cdef class SimpleReactor(ReactionSystem):
                reaction_rate = k * C[inet[j, 0]] * C[inet[j, 1]] * C[inet[j, 2]]
            network_leak_rates[j] = reaction_rate

+        if self.const_spc_indices is not None:
+            for spcIndice in self.const_spc_indices:


Use underscores for variable names, perhaps spc_index?

mliu49 · 2019-10-08T20:08:39Z

rmgpy/tools/simulate.py

        elif rmg.uncertainty is not None:
            rmg.verbose_comments = True
            rmg.load_database()

+	# Store constant species indices


Indentation should match the following line.

mliu49 · 2019-10-08T20:09:49Z

rmgpy/solver/simple.pyx

+    def get_const_spc_indices (self, coreSpecies):
+        "Allow to identify constant Species position in solver"
+        for spc in self.const_spc_names:
+            if self.const_spc_indices is None: #initialize once the list if constant SPC declared


Sorry, this is quite picky, but comments should have two spaces before the # and one space after:

if self.const_spc_indices is None: # initialize

rgillis8 · 2019-10-08T21:36:47Z

I think that this solves all the problems mentioned by Max? Let me know if you see any other dumb mistakes. The unit test works by comparing the number of resonance structures generated for S1SSS1. Previously it made 34, but elminating the S#S has reduced this number to 10.

mliu49

Thanks for making all of the changes. I made one more comment. You can also rebase on to master after fixing it.

mliu49 · 2019-10-09T13:51:21Z

rmgpy/molecule/resonanceTest.py

@@ -1419,3 +1419,11 @@ def test_surface_o(self):
 1 X u0 p0 c0 {2,D}
 2 O u0 p2 c0 {1,D}"""))
        self.assertEquals(len(mol_list), 1)
+
+    def sulfur_triple_bond(self):


I think tests have to start with test in order to be recognized and run as a test.

…hat do not contain charged atoms

…tiplebond_paths and find_adj_lone_pair_multiple_bond_delocalization_paths

…ultiple bond resonance pathways

rgillis8 · 2019-10-09T20:55:46Z

Ok. I think this resolves the remaining issues. It should be rebased to current master.

rgillis8 requested review from xiaoruiDong and lily90502 October 4, 2019 18:15

xiaoruiDong added Topic: Sulfur Topic: Thermo Twin RMG-database PR labels Oct 4, 2019

mliu49 reviewed Oct 4, 2019

View reviewed changes

xiaoruiDong reviewed Oct 4, 2019

View reviewed changes

rgillis8 force-pushed the DMSOxyChanges branch from 1b97fb6 to 041c19e Compare October 7, 2019 12:10

xiaoruiDong previously approved these changes Oct 8, 2019

View reviewed changes

lily90502 reviewed Oct 8, 2019

View reviewed changes

mliu49 requested changes Oct 8, 2019

View reviewed changes

rgillis8 dismissed xiaoruiDong’s stale review via bfd40e7 October 8, 2019 21:22

rgillis8 force-pushed the DMSOxyChanges branch from 5075f1e to bfd40e7 Compare October 8, 2019 21:22

rgillis8 requested a review from mliu49 October 9, 2019 13:38

mliu49 reviewed Oct 9, 2019

View reviewed changes

rgillis8 added 5 commits October 9, 2019 12:20

When estimating thermochemistry from GAV prioritize resonance forms t…

088cad7

…hat do not contain charged atoms

Allows constant concentration species in simple gas phase reactors

42ab29c

Allows valence 12 sulfur species, i.e. DMSO2

3987360

prevents resonance generation of S#S structures by find_lone_pair_mul…

27a0392

…tiplebond_paths and find_adj_lone_pair_multiple_bond_delocalization_paths

Adds a unit test to confirm that S#S is not formed by the lone pair m…

f61752e

…ultiple bond resonance pathways

rgillis8 force-pushed the DMSOxyChanges branch from bfd40e7 to f61752e Compare October 9, 2019 16:21

mliu49 approved these changes Oct 9, 2019

View reviewed changes

mliu49 merged commit 59c93db into master Oct 9, 2019

mliu49 deleted the DMSOxyChanges branch October 9, 2019 22:32

mliu49 mentioned this pull request Nov 26, 2019

RMG v3.0.0 Release Planning #1830

Closed

mliu49 mentioned this pull request Dec 16, 2019

RMG-Py v3.0.0 Release #1852

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dms oxy changes #1751

Dms oxy changes #1751

rgillis8 commented Oct 4, 2019

rgillis8 commented Oct 4, 2019

mliu49 Oct 4, 2019

xiaoruiDong left a comment

xiaoruiDong Oct 4, 2019

xiaoruiDong Oct 4, 2019

xiaoruiDong Oct 4, 2019

xiaoruiDong Oct 4, 2019

xiaoruiDong Oct 4, 2019

rgillis8 commented Oct 7, 2019

mliu49 commented Oct 7, 2019

rgillis8 commented Oct 7, 2019

alongd commented Oct 8, 2019

rgillis8 commented Oct 8, 2019

codecov bot commented Oct 8, 2019 •

edited

Loading

xiaoruiDong commented Oct 8, 2019

lily90502 Oct 8, 2019

mliu49 left a comment

mliu49 Oct 8, 2019

mliu49 Oct 8, 2019

mliu49 Oct 8, 2019

mliu49 Oct 8, 2019

mliu49 Oct 8, 2019

mliu49 Oct 8, 2019

mliu49 Oct 8, 2019

mliu49 Oct 8, 2019

mliu49 Oct 8, 2019

rgillis8 commented Oct 8, 2019

mliu49 left a comment

mliu49 Oct 9, 2019

rgillis8 commented Oct 9, 2019

		self.const_spc_indices = None
		self.const_spc_names = const_spc_names #store index of constant species

Dms oxy changes #1751

Dms oxy changes #1751

Conversation

rgillis8 commented Oct 4, 2019

Motivation or Problem

Description of Changes

Testing

Reviewer Tips

rgillis8 commented Oct 4, 2019

Choose a reason for hiding this comment

xiaoruiDong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgillis8 commented Oct 7, 2019

mliu49 commented Oct 7, 2019

rgillis8 commented Oct 7, 2019

alongd commented Oct 8, 2019

rgillis8 commented Oct 8, 2019

codecov bot commented Oct 8, 2019 • edited Loading

Codecov Report

xiaoruiDong commented Oct 8, 2019

Choose a reason for hiding this comment

mliu49 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgillis8 commented Oct 8, 2019

mliu49 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgillis8 commented Oct 9, 2019

codecov bot commented Oct 8, 2019 •

edited

Loading