Added Chlorine as a new reactive element in RMG #1310

alongd · 2018-03-07T02:29:38Z

This PR allows Chlorine to be reactive in RMG. Only valance 1 is considered, and no hypervalance atomTypes were defined.
A respective atomType test was added, and the documentation was changed accordingly.
I also made a small change to the isomorphismTest which otherwise insists that Cl should be charged.

This PR will soon be accompanied by a sister -db PR with relevant kinetics and thermo data by @zjburas. They should be merged together, but this branch can principally be reviewed.

codecov · 2018-03-07T02:54:49Z

Codecov Report

Merging #1310 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1310      +/-   ##
==========================================
- Coverage   43.05%   43.03%   -0.02%     
==========================================
  Files         169      169              
  Lines       28342    28365      +23     
  Branches     5535     5538       +3     
==========================================
+ Hits        12203    12208       +5     
- Misses      15302    15321      +19     
+ Partials      837      836       -1

Impacted Files	Coverage Δ
rmgpy/cantherm/statmech.py	`20.67% <ø> (ø)`	⬆️
rmgpy/molecule/atomtype.py	`0% <0%> (ø)`	⬆️
rmgpy/molecule/group.py	`0% <0%> (ø)`	⬆️
rmgpy/reaction.py	`0% <0%> (ø)`	⬆️
rmgpy/molecule/element.py	`0% <0%> (ø)`	⬆️
rmgpy/molecule/molecule.py	`0% <0%> (ø)`	⬆️
rmgpy/data/kinetics/family.py	`58.19% <0%> (+0.4%)`	⬆️

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 f5b7467...f317c69. Read the comment docs.

mliu49 · 2018-03-07T22:57:17Z

@rwest mentioned that he was working on chlorine in #717. Are there any updates on the status of that?

and rearranged He/N2/Ar for better readability

alongd · 2018-03-13T13:59:46Z

@rwest , regarding @mliu49's comment above, are there any chlorine additions you'd like to add to either this PR or ReactionMechanismGenerator/RMG-database#257? Or shall we continue with these as they are?

rwest · 2018-03-13T14:59:37Z

Sorry, I've not been keeping on top of this or ReactionMechanismGenerator/RMG-database#257. Here's a brief recap.

Our work was done by @faribas and can be found on her halogens branches. If she still receives github notifications, she can correct what I get wrong...

RMG-Py: master...faribas:halogens
Database: ReactionMechanismGenerator/RMG-database@master...faribas:halogens

Because this forked three years ago (March 2015) it's probably hard to merge them directly, but there may be value in copying across what she did.

Thermochemistry:
as well as adding some thermo libraries and Benson's group additivity values for chlorinated hydrocarbons, she found she had to calculate her own hydrogen bond increment (HBI) values in order to estimate radicals containing chlorines. Next-nearest-neighbour effects were required to get decent estimates. She derived these from her own quantum calculations.

Reactions:
She allowed Cl to react as the radical for the two families H abstraction and radical recombination, and created new reaction families for H-Cl or Cl-Cl insertion into a multiple bond and Cl abstraction. Training rates were from the literature.

More details are in chapter 3 of her PhD thesis at http://hdl.handle.net/2047/D20213055

Also worth mentioning is that I have just received a grant to work on adding halogens to RMG (https://www.nsf.gov/awardsearch/showAward?AWD_ID=1751720), so would love to start a conversation about the best way to do this! It will be @skrsna working on that project, starting soon. Perhaps we can all meet.

alongd · 2018-03-13T15:44:19Z

Congrats on the grant! I love that this includes training undergrads in this kind of problem solving.

Other stakeholders here are @zjburas (currently in CA), and @sarahkha from the GreenGroup implementing iodine.

I'll look at @faribas's -Py repo, @zjburas, could you take a look at the -db repo?

zjburas · 2018-03-13T15:59:55Z

Sure @alongd , I can take a look at the -db repo.

Congratulations @rwest on the grant. My immediate interest in halogen chemistry is pretty narrow: I just want to use RMG as a tool to account for chlorine side-chemistry during chlorine initiated oxidation experiments from 300-1000 K. For that purpose, what @faribas already did is probably sufficient. Hopefully we can talk sometime.

amarkpayne

Great PR Alon! I have noted a couple of quick things that need fixing, but this PR should be ready for merging soon after these quick fixes.

The only other thing may be to add cases with chlorine to the unit tests for group.py and molecule.py, but this is probably not necessary.

amarkpayne · 2018-03-13T22:18:48Z

documentation/source/reference/molecule/atomtype.rst

 ``S6td``        sulfur atom with no lone pairs (valance 6), one triple bond, one double bond and up to one single bond
 ``S6tt``        sulfur atom with no lone pairs (valance 6) and two triple bonds
 ``S6tdc``       charged sulfur atom with no lone pairs (valance 6), one to two triple bonds, up to two double bonds, and up to four single bonds
 =============== ==============================================================================================================================================================


You need to delete line 118 for the table to format properly. Otherwise, it tries to end the table early

amarkpayne · 2018-03-13T22:21:09Z

documentation/source/users/rmg/database/introduction.rst

 +----------+-------------------+------------------------------------------------------------------------------------------------------------------+
 |S6tdc     |Sulfur             |No lone pairs, one to two triple bonds, up to two double bonds, up to four single bonds, charged -1/-1            |
 +----------+-------------------+------------------------------------------------------------------------------------------------------------------+
+|Cl1s      |Chlorine           |Three lone pairs, zero to one single bonds                                                                        |


Should you add an entry for Cl as well?

amarkpayne · 2018-03-13T22:30:16Z

rmgpy/cantherm/statmech.py

-            'C#C': -0.64,  'C#N': -0.89,  'C-S': 0.43,   'O=S': -0.78, 'S-H': 0.0,   'C-N': -0.13, # Table IX: Petersson GA (1998) J. of Chemical Physics, DOI: 10.1063/1.477794 
-            'N-H': -0.42,  'N=O': 1.11,   'N-N': -1.87,  'N=N': -1.58, 'N-O': 0.35, #Table 2: Ashcraft R (2007) J. Phys. Chem. B; DOI: 10.1021/jp073539t
-            'N#N': -2.0,   'O=O': -0.2,   'H-H': 1.1, # Unknown source
+            'C-C': -0.495,'C-H': -0.045,'C=C': -0.825,'C-O': 0.378,'C=O': 0.743,'O-H': -0.423,  #Table2: Paraskevas, PD (2013). Chemistry-A European J., DOI: 10.1002/chem.201301381


It looks like your editor changed some of the spacing here

That was actually me, trying to make the lines a bit shorter (now with the 'C-Cl', 'C-F' additions, the second line was too long). It still functions good and it's readable, let me know if you think otherwise.

Gotcha, this is fine by me then, just making sure

amarkpayne · 2018-03-13T22:44:30Z

rmgpy/molecule/group.py

+        self.siliconCount = 0
        self.radicalCount  = 0
        for atom in self.vertices:
            if len(atom.atomType) == 1:


This is outside the scope of your commit, but what should happen if len(atom.atomType) != 1?

My understanding is that since it is a Group and not a Molecule, an "atom" can be defined as 1 [C,N,O] ux px cx {2,S}, in which case we can't fingerprint it and we correctly skip this condition. I think this also means we're missing out on cases like 1 [Cs,Cd] ux px cx {2,S} where we could fingerprint, but don't (for that atom in the Group). However, this should be working smoothly for Molecules.

That makes sense. I wonder if there is a way to catch cases like 1 [Cs,Cd] ux px cx {2,S} and if that would be useful. If so this is something for another PR though.

amarkpayne · 2018-03-13T22:46:51Z

rmgpy/molecule/group.py

+                isChlorine = atomType.equivalent(chlorine)
+                isSilicon  = atomType.equivalent(silicon)
+                sum_is_atom = isCarbon + isNitrogen + isOxygen + isSulfur + isChlorine + isSilicon
+                if sum_is_atom == 1:


What happens if sum_is_atom != 1? I can't imagine this happening, but maybe I am missing a subtlety here. I would think that this would either always evaluate to True (and thus it is not needed) or we need some sort of error handling if it does occur.

I think this is related to the previous comment: an "atom" in a Group could be defined as 1 [C,N,O] ux px cx {2,S}. In this case, atomType = [C,N,O], and .equivalent will return True for more than one isAtom. In this case, sum_is_atom will be 3.
I agree that it seems unnecessary (now that I rethink it) since it's already under the if len(atom.atomType) == 1: condition discussed above. We could either keep it to keep the same functionality as before in case I'm also missing a subtlety, or remove. I'll check.

amarkpayne · 2018-03-13T22:49:46Z

rmgpy/molecule/molecule.pxd


+    cpdef bint isFluorine(self)
+
+    cpdef bint isSilicon(self)


Make a note in the commit that isSilicon was added to cython file as well, good catch!

amarkpayne · 2018-03-13T22:50:22Z

rmgpy/molecule/molecule.py


+    def isFluorine(self):
+        """
+        Return ``True`` if the atom represents an oxygen atom or ``False`` if


Change doc string element name to "fluorine"

amarkpayne · 2018-03-13T22:51:49Z

rmgpy/molecule/molecule.py


+    def isChlorine(self):
+        """
+        Return ``True`` if the atom represents an sulfur atom or ``False`` if


Same here, but for "chlorine"

amarkpayne · 2018-03-13T22:52:18Z

rmgpy/molecule/molecule.py

+
+    def isIodine(self):
+        """
+        Return ``True`` if the atom represents an sulfur atom or ``False`` if


Same here, but for "Iodine"

amarkpayne · 2018-03-13T22:54:02Z

rmgpy/molecule/group.py

        carbon   = atomTypes['C']
        nitrogen = atomTypes['N']
        oxygen   = atomTypes['O']
+        # flourine = atomTypes['F']


I think we should either go ahead and add fluorine and iodine atom types in this PR, or remove this commented line until we are ready.

alongd · 2018-03-14T00:17:56Z

Thanks for the timely review, @amarkpayne! I addressed all comments but one in fixup commits. I'll double check the implications of removing the if sum_is_atom == 1: condition.

amarkpayne

Looks good! I am looking into the if sum_is_atom == 1: issue as well, though so far it appears that the unit tests do fail without this. Given this I think this PR is good to go. Go ahead and auto-squash the fixup commits, and I'll merge this in

alongd · 2018-03-14T00:42:22Z

I just saw your comment. I pushed an optional solution, if it won't do the trick I'll squash w/o it

Table IX: Petersson GA (1998) J. of Chemical Physics, DOI: 10.1063/1.477794

Also added isSilicon to molecule.pxd, which eas missing

alongd · 2018-03-14T02:53:32Z

We believe this PR is a comprehensive example for adding new reactive elements into RMG.
See also #1206 for sulfur and #888 for nitrogen
Also, consider elaborating known SMILES, see #1325

alongd added Type: Feature Status: Ready for Review PR is complete and ready to be reviewed Twin RMG-database PR Topic: Chlorine labels Mar 7, 2018

alongd requested a review from amarkpayne March 7, 2018 02:29

alongd force-pushed the chlorine branch from 524edb0 to ed323a8 Compare March 7, 2018 02:32

zjburas mentioned this pull request Mar 12, 2018

Added Kinetics Data for Hydrogen Abstraction by Chlorine Atom ReactionMechanismGenerator/RMG-database#257

Merged

Added Cl1s chlorine atomType

e2bd018

and rearranged He/N2/Ar for better readability

alongd force-pushed the chlorine branch from ed323a8 to d94af40 Compare March 13, 2018 13:53

alongd force-pushed the chlorine branch from ea06060 to be5dd6a Compare March 13, 2018 17:33

amarkpayne requested changes Mar 13, 2018

View reviewed changes

alongd force-pushed the chlorine branch from be5dd6a to 4f6b286 Compare March 14, 2018 00:15

amarkpayne previously approved these changes Mar 14, 2018

View reviewed changes

alongd dismissed amarkpayne’s stale review via 599e829 March 14, 2018 00:40

alongd added 7 commits March 13, 2018 20:50

Added Chlorine atomType to documentation

444d908

Added a chlorine atomType Test

7495ad0

Avoid generating a charged chlorine atomType in isomorphismTest

7cd5cf5

Added C-Cl and C-F to CBS-QB3 BAC

6ed0c14

Table IX: Petersson GA (1998) J. of Chemical Physics, DOI: 10.1063/1.477794

Added F, I to element.py

4288bec

Added Cl, Si to group updateFingerprint()

87dc475

Added is F/Cl/I to Atom in molecule.py

7758183

Also added isSilicon to molecule.pxd, which eas missing

alongd force-pushed the chlorine branch from 599e829 to 7758183 Compare March 14, 2018 00:51

amarkpayne removed the Status: Ready for Review PR is complete and ready to be reviewed label Mar 14, 2018

alongd force-pushed the chlorine branch from 5c84603 to f79de55 Compare March 14, 2018 01:31

Added Cl and Si to generatePairs() in reaction.py

f317c69

alongd force-pushed the chlorine branch from f79de55 to f317c69 Compare March 14, 2018 02:20

alongd added the Status: Accepted This PR is approved and ready to be merged label Mar 14, 2018

amarkpayne approved these changes Mar 14, 2018

View reviewed changes

amarkpayne merged commit 73d4706 into master Mar 14, 2018

amarkpayne deleted the chlorine branch March 14, 2018 02:52

amarkpayne removed the Status: Accepted This PR is approved and ready to be merged label Mar 14, 2018

alongd changed the title ~~Added Chlorine Cl1s atomType~~ Added Chlorine as a new reactive element in RMG Mar 14, 2018

sarahkha mentioned this pull request Mar 16, 2018

Adding Iodine as a new reactive element in RMG #1321

Merged

alongd mentioned this pull request Nov 7, 2020

Add phosphorus atom types #2037

Merged

Added Chlorine as a new reactive element in RMG #1310

Added Chlorine as a new reactive element in RMG #1310

Uh oh!

Conversation

alongd commented Mar 7, 2018

Uh oh!

codecov bot commented Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mliu49 commented Mar 7, 2018

Uh oh!

alongd commented Mar 13, 2018

Uh oh!

rwest commented Mar 13, 2018

Uh oh!

alongd commented Mar 13, 2018

Uh oh!

zjburas commented Mar 13, 2018

Uh oh!

amarkpayne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alongd commented Mar 14, 2018

Uh oh!

amarkpayne left a comment

Choose a reason for hiding this comment

Uh oh!

alongd commented Mar 14, 2018

Uh oh!

alongd commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Mar 7, 2018 •

edited

Loading

alongd commented Mar 14, 2018 •

edited

Loading