-
Notifications
You must be signed in to change notification settings - Fork 230
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
Adding Fluorine as a new element in RMG #1543
Conversation
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 @oscarwumit! Please see the minor comments below
rmgpy/reaction.py
Outdated
|
||
# Sort the reactants and products by C/O/N/S numbers | ||
reactants = [(carbon, oxygen, nitrogen, silicon, sulfur, chlorine, iodine, reactant) for carbon, oxygen, nitrogen, silicon, sulfur, chlorine, iodine, reactant | ||
reactants = [(carbon, oxygen, nitrogen, silicon, sulfur, chlorine, iodine, fluorine, reactant) for carbon, oxygen, nitrogen, silicon, sulfur, chlorine, iodine, reactant |
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.
I think this should be:
reactants = [(carbon, oxygen, nitrogen, silicon, sulfur, chlorine, iodine, fluorine, reactant) for carbon, oxygen, nitrogen, silicon, sulfur, chlorine, iodine, reactant | |
reactants = [(carbon, oxygen, nitrogen, silicon, sulfur, chlorine, iodine, fluorine, reactant) for carbon, oxygen, nitrogen, silicon, sulfur, chlorine, iodine, fluorine, reactant |
(same for products
below)
rmgpy/reaction.py
Outdated
|
||
# Sort the reactants and products by C/O/N/S numbers | ||
reactants = [(carbon, oxygen, nitrogen, silicon, sulfur, chlorine, iodine, reactant) for carbon, oxygen, nitrogen, silicon, sulfur, chlorine, iodine, reactant | ||
reactants = [(carbon, oxygen, nitrogen, silicon, sulfur, chlorine, iodine, fluorine, reactant) for carbon, oxygen, nitrogen, silicon, sulfur, chlorine, iodine, reactant | ||
in zip(reactantCarbons,reactantOxygens,reactantNitrogens,reactantSilicons,reactantSulfurs,reactantChlorines, reactantIodines, reactants)] |
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.
I think reactantFluorines
should be added to the zip here, same for products below
reactantIodines = [sum([1 for atom in reactant.molecule[0].atoms if atom.isIodine()]) for reactant in reactants] | ||
productIodines = [sum([1 for atom in product.molecule[0].atoms if atom.isIodine()]) for product in products ] | ||
reactantFluorines = [sum([1 for atom in reactant.molecule[0].atoms if atom.isFluorine()]) for reactant in reactants] | ||
productFluorines = [sum([1 for atom in product.molecule[0].atoms if atom.isFluorine()]) 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.
Do we have the isFluorine()
method already defined?
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.
Which file should I modify to define this function?
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.
We do already have isFluorine()
, it's here
Codecov Report
@@ Coverage Diff @@
## master #1543 +/- ##
==========================================
+ Coverage 42% 42.01% +<.01%
==========================================
Files 165 165
Lines 27867 27873 +6
Branches 5680 5680
==========================================
+ Hits 11706 11711 +5
- Misses 15375 15377 +2
+ Partials 786 785 -1
Continue to review full report at Codecov.
|
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.
Awesome, thanks for this PR, @oscarwumit!
Motivation or Problem
Adding Fluorine as an atomtype to RMG.
Description of Changes
Files related to atomtypes are changed.
Testing
Added Fluorine atomtype test.