Skip to content

Conversation

@sarahkha
Copy link
Member

Added I1s atomType along with respective atom test following the same procedure as Chlorine #1310

@codecov
Copy link

codecov bot commented Mar 16, 2018

Codecov Report

Merging #1321 into master will increase coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
rmgpy/reaction.py 0% <0%> (ø) ⬆️
rmgpy/molecule/group.py 0% <0%> (ø) ⬆️
rmgpy/molecule/atomtype.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 c45ca17...98bce38. Read the comment docs.

Copy link
Member

@alongd alongd left a 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
Copy link
Member

@alongd alongd Mar 16, 2018

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)

'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',

Copy link
Member

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?

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 ]
Copy link
Member

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


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]))
Copy link
Member

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?

@sarahkha
Copy link
Member Author

Thanks @alongd
All comments were addressed and ready for review.

Copy link
Member

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

@alongd alongd merged commit d02ed26 into master Mar 20, 2018
@alongd alongd deleted the iodineMonoval branch March 20, 2018 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants