Add phosphorus atom types #2037

yunsiechung · 2020-11-06T22:07:12Z

Motivation or Problem

Phosphorus is currently not supported by RMG. Phosphorus should be added for future use.

(My own motivation in more detail
My primary purpose of adding phosphorus in RMG is so that I can make solvation thermo groups for phosphorus containing molecules later. I did not include the phosphorus solvation thermo in this pull request because I actually need to make more extensive changes in solvation.py in order to add phosphorus solvation thermo group. So I figured it would be better to divide it into two pull requests, and use this one to only add Phosphorus atomtypes to make reviewing process easier.)

Description of Changes

Atomtypes for phosphorus are added so RMG can generate the molecules containing phosphorus.

For each atomtype, sample compounds that contain the corresponding atomtype are also added as comments.
Nearly all the sample compounds are the ones that could be found from PubChem molecule search. (This is to make sure that I'm not adding non-existing atomtypes for phosphorus).

For some atomtypes such as P1sc, P1dc, P5ddc, and etc., the sample compounds given in comments are ionic ones and neutral ones are very unlikely to be found in real life. Although RMG does not support ionic compounds now, these atomtypes are still added in this commit for potential use in the future.

Many phosphorus atomtypes are generated by referencing to nitrogen ones because they share many similar types of bonding. Although the aromaticity of heterocycles containing phosphorus is much smaller than other heterocyclic aromatics, I still added aromatic phosphorus atomtypes (P3b, P5b, P5bd). (Some study on aromaticity of phosphorus heterocycles can be found on doi: 10.1021/cr990321)

I have added 20 atomtypes for phosphorus. If any of these atomtypes doesn't make sense, please let me know and I can just comment them out.

A set of actions (increment_bond, decrement_bond, form_bond, etc.) has been added for each atomtype.
Phosphorus is added to make_sample_atom() and make_sample_molecule() functions in rmgpy/molecule/group.py. Relevant unit test has been added.
Added phosphorus to isomorphismTest.py. In this test, infeasible phosphorus groups (+1 partial charge with 1 lone pair) are forbidden.
Phosphorus is added to sorting key for generate_pairs() in rmgpy/reaction.py. Relevant unit test has been added to confirm that RMG can now make reactions involving phosphorus.
Documentation for phosphorus atomType is added

Testing

A unit test for Atom.is_phosphorus() is added.

A unit test, test_phosphorus_types, is added to test all phosphorus atomtypes.

Phosphorus containing molecules have been added to rmgpy/molecule/groupTest.py unit tests.

Phosphorus has been added to rmgpy/molecule/isomorphismTest.py unit test.

A unit test, test_phosphorus_reaction_pairs(), is added to rmgpy/reactionTest.py to test that reaction pairs can be correctly generated for the reaction involving phosphorus species.

The modified documentation has been tested with make html command to make sure the documentation can be generated correctly.

Reviewer Tips

You can run unit tests and also try generating some compounds and reactions containing phosphorus.

codecov · 2020-11-07T00:34:19Z

Codecov Report

Merging #2037 (40fd4f3) into master (b1f50db) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2037   +/-   ##
=======================================
  Coverage   47.54%   47.54%           
=======================================
  Files          88       88           
  Lines       23300    23300           
  Branches     6061     6061           
=======================================
  Hits        11079    11079           
+ Misses      11063    11062    -1     
- Partials     1158     1159    +1

Impacted Files	Coverage Δ
arkane/kinetics.py	`12.24% <0.00%> (ø)`
rmgpy/molecule/draw.py	`52.74% <0.00%> (ø)`

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 b1f50db...40fd4f3. Read the comment docs.

alongd · 2020-11-07T11:12:33Z

I think there might be some more considerations when adding new atom types, see the commits in #1310

yunsiechung · 2020-11-08T00:10:07Z

I think there might be some more considerations when adding new atom types, see the commits in #1310

Thanks, @alongd! I forgot to take more things into considerations.

It looks like I have to also change rmgpy/molecule/group.py, rmgpy/molecule/group.pxd , rmgpy/molecule/molecule.pyx, rmgpy/molecule/isomorphismTest.py, and documentation/source/reference/molecule/atomtype.rst.

I'm not sure whether I should modify rmgpy/reaction.py to make Phosphorus reactive. Should I include phosphorus to def generate_pairs in rmgpy/reaction.py ?

So my primary purpose of adding phosphorus in RMG is so that I can make solvation thermo groups for phosphorus containing molecules in the future. I haven't considered any reactions involving phosphorus containing molecules.

alongd · 2020-11-08T06:49:52Z

@yunsiechung, was this PR closed by mistake?
I we can add all the necessary code to make it reactive. I think we need to modify def_generate_pairs as well, but I haven't looked at it recently. Let me or any of the reviewers tagged here now if we can assist.

yunsiechung · 2020-11-08T16:42:22Z

@aelong Sorry, I closed it by mistake. I reopened the PR.

Thanks for help. I will make all the necessary changes and make Phosphorus reactive as well.
I might miss a few things though, so if anyone spots out any missing parts, let me know!

yunsiechung · 2020-11-10T00:07:11Z

@alongd I believe I made all the necessary changes for adding Phosphorus. I have listed all the changes I have made in my very top comment under 'Description of Changes'.
I didn't have to make too many changes to rmgpy/molecule/group.py like it had to be for #1310 because the file has been changed a lot since then.

( I decided not to remove N3sc atomType in this PR (#1233). I will make it as a separate PR)

@mjohnson541 @amarkpayne I would really appreciate it if any of you could also review this PR when you have time.

mjohnson541

I think this looks fine except we need BDE values for phosphorus bonds in rmgpy/molecule/element.py.

yunsiechung · 2020-11-19T18:05:24Z

@mjohnson541 Thanks for your comment! I just have a few questions regarding BDE values in RMG.

Do I need to add BDE values for bonds between phosphorus and all the other elements in bde_elements list? (bde_elements = ['C', 'N', 'H', 'O', 'S', 'Cl', 'Si']) It looks like the bde_dict doesn't cover all bond combinations between elements in the bde_elements. For example, in bde_dict, the BDE between 'H' and 'Si' is missing.

Also, I'm not sure which elements are supposed to be added to bde_elements list. It looks like I, F, Br are able to react but they are not added to bde_elements. Is it because I, F, Br are always used as dummy elements in RMG?

mjohnson541 · 2020-11-19T19:21:07Z

So essentially this is required for using Blower's Masel rate coefficient forms that automatic trees use. Currently I believe rate estimation for R_Recombination will error if you try to estimate a reaction that breaks or forms a bond that's BDE pair isn't in the dictionary.

yunsiechung · 2020-11-19T19:23:07Z

@mjohnson541 I see, thanks!
When I add BDE values for phosphorus, I will also add missing ones if I find them.

xiaoruiDong

Thank @yunsiechung, thank you for adding support of phosphorous into RMG. I went through your codes and they look good. I just have a minor concern about the atom type part, and please see the comment.

xiaoruiDong · 2020-11-23T16:42:54Z

rmgpy/molecule/atomtype.py

+ATOMTYPES['P'].set_actions(increment_bond=['P'], decrement_bond=['P'], form_bond=['P'], break_bond=['P'], increment_radical=['P'], decrement_radical=['P'], increment_lone_pair=['P'], decrement_lone_pair=['P'])
+ATOMTYPES['P0sc'].set_actions(increment_bond=[], decrement_bond=[], form_bond=['P0sc'], break_bond=['P0sc'], increment_radical=['P0sc'], decrement_radical=['P0sc'], increment_lone_pair=[], decrement_lone_pair=['P1s', 'P1sc'])
+ATOMTYPES['P1s'].set_actions(increment_bond=['P1dc'], decrement_bond=[], form_bond=['P1s'], break_bond=['P1s'], increment_radical=['P1s'], decrement_radical=['P1s'], increment_lone_pair=['P0sc'], decrement_lone_pair=['P3s'])
+ATOMTYPES['P1sc'].set_actions(increment_bond=['P1dc'], decrement_bond=[], form_bond=['P1sc'], break_bond=['P1sc'], increment_radical=['P1sc'], decrement_radical=['P1sc'], increment_lone_pair=['P0sc'], decrement_lone_pair=['P3s'])


Not sure whether this will be a problem. Since P3sc is no longer included, decrement_lone_pair=['P3s'] for atom type P1sc. But P1sc has a charge of -1, while P3s has a charge of 0. It seems to me that the charge will be conflicted. I guess we can either keep the P3sc or modify P3s's charge attribute?

Okay, it seems like the charge will be updated, after applying decrement_lone_pair()

I was looking at Nitrogen and Sulfur AtomType, and they have something similar to that (charged AtomType has non-charged AtomType for decrement_lone_pair() and increment_lone_pair()), so I thought it would be alright.
So it's okay as it is?

yunsiechung · 2020-11-24T02:52:58Z

@mjohnson541 I added BDE for Phosphorus as well as F, Br, and I.
I re-arranged the bde_dict a bit to make it look cleaner, and I triple-checked that all the values are correct.

Phosphorus is added to PeriodicSystem class object so phosphorus can supported by RMG

A method to check whether the atom is a phosphorus atom or not is added

A unit test to test the Atom.is_phosphorus() method has been added

Atomtypes for Phosphorus are created to support phosphorus compounds in RMG. 20 atomtypes are added for phosphorus, and most of them are created by referencing to nitrogen atomtypes since nitrogen and phosphorus share many similar bond types. For each atomtype, SMILES strings of sample compounds have been added as comments. Atomtype attributes such as 'increment_bond', 'decrement_bond', and etc. are also created by referncing to those of nitrogen. Notice that phosphorus has several new atomtypes that do not exist for nitrogen, including P5s, P5d, P5td, etc.

Unit test for phosphorus atomtype, test_phosphorus_types, is added. This tests all phosphorus atomtypes.

Phosphorus is added to `make_sample_molecule` method. Br was missing from here, so Br is also added. Positively charged and negatively charged phosphorus atomtypes are added to `make_sample_molecule`. S4sc was missing from `negative_charged` so it is also added.

In `test_make_sample_molecule`, I added two more cases with phosphorus for creating positively and negatively charged species. Also, the test species in `test_get_element_count` is slightly modified to test whether phosphorus can be properly counted as well.

Phosphorus is added to isomorphismTest. For creating a molecule using `create_molecule`, it is forbidden to create a molecule containing the phosphorus with +1 partial charge and 1 lone pair because this is chemically infeasible. Creating such molecules raises AtomTypeError, not InvalidAdjacenctListError, so additional lines are added in `create_molecule` to pass creating such molecules.

Phosphorus is added to get_sorting_key() in generate_pairs().

A unit test `test_phosphorus_reaction_pairs()` is added to test whether reaction pairs can be correctly generated for the reaction involving phosphorus species.

Documentation for Phosphorus atomType is added

Bond dissociation energies for P, F, Br, and I are added. The source is added as comment. The bond disccosiation energies in `bde_dict` are a bit re-arranged. Some are missing because they could not be found (e.g. N-P, N-Br, N-I, etc). The information about reference state is also added as comment.

yunsiechung requested review from amarkpayne, mjohnson541 and xiaoruiDong November 6, 2020 22:07

yunsiechung closed this Nov 8, 2020

yunsiechung reopened this Nov 8, 2020

yunsiechung force-pushed the add_Phosphorus_atomType branch from 57efcc0 to bf84542 Compare November 9, 2020 23:45

yunsiechung force-pushed the add_Phosphorus_atomType branch from bf84542 to d12e137 Compare November 10, 2020 18:26

mjohnson541 requested changes Nov 19, 2020

View reviewed changes

xiaoruiDong reviewed Nov 23, 2020

View reviewed changes

yunsiechung force-pushed the add_Phosphorus_atomType branch from d12e137 to ee26f7a Compare November 23, 2020 20:44

mjohnson541 previously approved these changes Dec 1, 2020

View reviewed changes

yunsiechung added 9 commits December 1, 2020 14:01

add phosphorus to PeriodicSystem class object

de2f020

Phosphorus is added to PeriodicSystem class object so phosphorus can supported by RMG

Add Atom.is_phosphorus() method

2224f7d

A method to check whether the atom is a phosphorus atom or not is added

A unittest for Atom.is_phosphorus() is added

5001e36

A unit test to test the Atom.is_phosphorus() method has been added

add unit tests for phosphorus atomtype

4dd7a4d

Unit test for phosphorus atomtype, test_phosphorus_types, is added. This tests all phosphorus atomtypes.

Add Phosphorus to sorting key for generate_pairs()

783a46b

Phosphorus is added to get_sorting_key() in generate_pairs().

yunsiechung added 3 commits December 1, 2020 14:01

Add unit test for test_phosphorus_reaction_pairs()

4ae759c

A unit test `test_phosphorus_reaction_pairs()` is added to test whether reaction pairs can be correctly generated for the reaction involving phosphorus species.

Added Phosphorus atomType to documentation

afe61f5

Documentation for Phosphorus atomType is added

yunsiechung dismissed mjohnson541’s stale review via 40fd4f3 December 1, 2020 19:02

yunsiechung force-pushed the add_Phosphorus_atomType branch from ee26f7a to 40fd4f3 Compare December 1, 2020 19:02

mjohnson541 approved these changes Dec 1, 2020

View reviewed changes

mjohnson541 merged commit 269daa9 into master Dec 1, 2020

mjohnson541 deleted the add_Phosphorus_atomType branch December 1, 2020 23:09

Add phosphorus atom types #2037

Add phosphorus atom types #2037

Conversation

yunsiechung commented Nov 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation or Problem

Description of Changes

Testing

Reviewer Tips

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

alongd commented Nov 7, 2020

Uh oh!

yunsiechung commented Nov 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alongd commented Nov 8, 2020

Uh oh!

yunsiechung commented Nov 8, 2020

Uh oh!

yunsiechung commented Nov 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjohnson541 left a comment

Choose a reason for hiding this comment

Uh oh!

yunsiechung commented Nov 19, 2020

Uh oh!

mjohnson541 commented Nov 19, 2020

Uh oh!

yunsiechung commented Nov 19, 2020

Uh oh!

xiaoruiDong left a comment

Choose a reason for hiding this comment

Uh oh!

xiaoruiDong Nov 23, 2020

Choose a reason for hiding this comment

Uh oh!

xiaoruiDong Nov 23, 2020

Choose a reason for hiding this comment

Uh oh!

yunsiechung Nov 23, 2020

Choose a reason for hiding this comment

Uh oh!

yunsiechung commented Nov 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yunsiechung commented Nov 6, 2020 •

edited

Loading

codecov bot commented Nov 7, 2020 •

edited

Loading

yunsiechung commented Nov 8, 2020 •

edited

Loading

yunsiechung commented Nov 10, 2020 •

edited

Loading