Skip to content
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

Include bond order in TopologyGraph #461

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

justinGilmer
Copy link
Contributor

PR Summary:

Foyer currently does not understand bond order in its TopologyGraph
representation and in its SMARTS grammar aside from number of connected
neighbors [C;X4], for example.

This PR adds bond order as an attribute when adding bonds in the
TopologyGraph, accepted options currently are:

"1" : single bonds
"2" : double bonds
"3" : triple bonds
"ar" : aromatic bonds
"am" : amide bonds
"un": unknown,
"du": dummy
"nc": not connected

The last 3 bond types ("un", "du", and "nc") are subject to change and
may not be here in the final iteration of this PR.

Support has been added when converting parmed structures, openff
topologies, as well as GMSO topologies.

PR Checklist


  • Includes appropriate unit test(s)
  • Appropriate docstring(s) are added/updated
  • Code is (approximately) PEP8 compliant
  • Issue(s) raised/addressed?

`Foyer` currently does not understand bond order in its `TopologyGraph`
representation and in its SMARTS grammar aside from number of connected
neighbors [C;X4], for example.

This PR adds bond order as an attribute when adding bonds in the
`TopologyGraph`, accepted options currently are:

"1"  : single bonds
"2"  : double bonds
"3"  : triple bonds
"ar" : aromatic bonds
"am" : amide bonds
"un": unknown,
"du": dummy
"nc": not connected

The last 3 bond types ("un", "du", and "nc") are subject to change and
may not be here in the final iteration of this PR.

Support has been added when converting parmed structures, openff
topologies, as well as GMSO topologies.
@@ -183,11 +203,22 @@ def from_openff_topology(cls, openff_topology):
element=element,
)

bond_type_dict = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be better implemented as an Enum.

@codecov
Copy link

codecov bot commented Oct 27, 2021

Codecov Report

Merging #461 (e031e05) into main (882fd32) will decrease coverage by 21.09%.
The diff coverage is 31.57%.

❗ Current head e031e05 differs from pull request most recent head dfb9825. Consider uploading reports for the commit dfb9825 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #461       +/-   ##
===========================================
- Coverage   69.55%   48.47%   -21.09%     
===========================================
  Files          16       17        +1     
  Lines        1662     1863      +201     
===========================================
- Hits         1156      903      -253     
- Misses        506      960      +454     

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants