-
Notifications
You must be signed in to change notification settings - Fork 250
Adding Iodine as a new reactive element in RMG #1321
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1321 +/- ##
==========================================
+ Coverage 42.33% 42.33% +<.01%
==========================================
Files 168 168
Lines 27196 27207 +11
Branches 5248 5249 +1
==========================================
+ Hits 11514 11519 +5
- Misses 14937 14944 +7
+ Partials 745 744 -1
Continue to review full report at Codecov.
|
alongd
left a comment
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 @sarahkha, this is a wonderful addition!
Please see my minor comments.
Also, what did we decide regarding adding the hypervalance iodine atomTypes already well-defined in a commented section?
| ``Cl1s`` chlorine atom with three lone pairs and zero to one single bonds | ||
| *Iodine atom types* | ||
| ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ||
| ``I`` iodine atom with any local bond structure |
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.
It might not look important at first glance, but for this table to look good in our online documentation the spacings have to be precise. Note that the descriptions for Iodine aren't aligned with the other descriptions (one blank space needs to be added in each line before the description)
rmgpy/molecule/atomtype.py
Outdated
| 'C','Ca','Cs','Csc','Cd','CO','CS','Cdd','Cdc','Ct','Cb','Cbf','C2s','C2sc','C2d','C2dc','C2tc', | ||
| 'N','N0sc','N1s','N1sc','N1dc','N3s','N3sc','N3d','N3t','N3b','N5sc','N5dc','N5ddc','N5dddc','N5t','N5tc','N5b','N5bd', | ||
| 'O','Oa','O0sc','O2s','O2sc','O2d','O4sc','O4dc','O4tc','O4b', | ||
|
|
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.
This is not because of the current PR, but there's an unnecessary blank line here. Could you remove it?
rmgpy/reaction.py
Outdated
| reactantChlorines = [sum([1 for atom in reactant.molecule[0].atoms if atom.isChlorine()]) for reactant in reactants] | ||
| productChlorines = [sum([1 for atom in product.molecule[0].atoms if atom.isChlorine()]) for product in products ] | ||
| reactantIodines = [sum([1 for atom in reactant.molecule[0].atoms if atom.isChlorine()]) for reactant in reactants] | ||
| productIodines = [sum([1 for atom in product.molecule[0].atoms if atom.isChlorine()]) for product in products ] |
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.
These should be spaced according to the above lines for better code readability
rmgpy/reaction.py
Outdated
|
|
||
| while len(reactants) > 1 and len(products) > 1: | ||
| self.pairs.append((reactants[-1][6], products[-1][6])) | ||
| self.pairs.append((reactants[-1][7], products[-1][7])) |
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.
Can you change [7] to [-1] here and below?
|
Thanks @alongd |
alongd
left a comment
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 for this wonderful contribution, @sarahkha! All changes look good.
Added I1s atomType along with respective atom test following the same procedure as Chlorine #1310