From 7ce6b7d4f87c09afc02495015b9bba54ee825509 Mon Sep 17 00:00:00 2001 From: Max Liu Date: Mon, 13 May 2019 16:51:41 -0400 Subject: [PATCH 1/4] Add failing unit test for interesting resonance bug Depending on order of atoms in adjlist, different resonance structures are generated. Root cause seems to be difference in aromaticity perception by RDKit. --- rmgpy/molecule/resonanceTest.py | 53 +++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/rmgpy/molecule/resonanceTest.py b/rmgpy/molecule/resonanceTest.py index 647cc7772d..4d96878983 100644 --- a/rmgpy/molecule/resonanceTest.py +++ b/rmgpy/molecule/resonanceTest.py @@ -1141,6 +1141,59 @@ def testFalseNegativePolycylicAromaticityPerception2(self): self.assertEqual(len(out), 7) self.assertTrue(any([m.isIsomorphic(aromatic) for m in out])) + @work_in_progress + def testInconsistentAromaticStructureGeneration(self): + """Test an unusual case of inconsistent aromaticity perception.""" + mol1 = Molecule().fromAdjacencyList(""" +multiplicity 2 +1 C u0 p0 c0 {2,S} {6,S} {11,S} {12,S} +2 C u0 p0 c0 {1,S} {3,D} {4,S} +3 C u0 p0 c0 {2,D} {5,S} {7,S} +4 C u0 p0 c0 {2,S} {7,D} {10,S} +5 C u1 p0 c0 {3,S} {8,S} {9,S} +6 C u0 p0 c0 {1,S} {8,D} {13,S} +7 C u0 p0 c0 {3,S} {4,D} {17,S} +8 C u0 p0 c0 {5,S} {6,D} {14,S} +9 C u0 p0 c0 {5,S} {10,D} {15,S} +10 C u0 p0 c0 {4,S} {9,D} {16,S} +11 H u0 p0 c0 {1,S} +12 H u0 p0 c0 {1,S} +13 H u0 p0 c0 {6,S} +14 H u0 p0 c0 {8,S} +15 H u0 p0 c0 {9,S} +16 H u0 p0 c0 {10,S} +17 H u0 p0 c0 {7,S} +""") + + mol2 = Molecule().fromAdjacencyList(""" +multiplicity 2 +1 C u0 p0 c0 {2,S} {6,S} {11,S} {12,S} +2 C u0 p0 c0 {1,S} {3,D} {4,S} +3 C u0 p0 c0 {2,D} {5,S} {7,S} +4 C u0 p0 c0 {2,S} {7,D} {9,S} +5 C u1 p0 c0 {3,S} {8,S} {10,S} +6 C u0 p0 c0 {1,S} {8,D} {13,S} +7 C u0 p0 c0 {3,S} {4,D} {16,S} +8 C u0 p0 c0 {5,S} {6,D} {15,S} +9 C u0 p0 c0 {4,S} {10,D} {17,S} +10 C u0 p0 c0 {5,S} {9,D} {14,S} +11 H u0 p0 c0 {1,S} +12 H u0 p0 c0 {1,S} +13 H u0 p0 c0 {6,S} +14 H u0 p0 c0 {10,S} +15 H u0 p0 c0 {8,S} +16 H u0 p0 c0 {7,S} +17 H u0 p0 c0 {9,S} +""") + + # These two slightly different adjlists should be the same structure + self.assertTrue(mol1.isIsomorphic(mol2)) + + # However, they give different resonance structures + res1 = generate_resonance_structures(mol1) + res2 = generate_resonance_structures(mol2) + self.assertEqual(res1, res2) + class ClarTest(unittest.TestCase): """ From fa588a2b04ffd5c40a6e2638dc03f69444bb620f Mon Sep 17 00:00:00 2001 From: Max Liu Date: Mon, 13 May 2019 16:53:37 -0400 Subject: [PATCH 2/4] Fix optimal arom res struct generation to try all structures Previously, would only attempt to aromatize structures with the max number of aromatic rings. New algorithm will try each potential number of aromatic rings, starting with the largest number, until it successfully aromatizes the molecule. --- rmgpy/molecule/resonance.py | 62 +++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 34 deletions(-) diff --git a/rmgpy/molecule/resonance.py b/rmgpy/molecule/resonance.py index c2d1f651e8..1fa1e4399e 100644 --- a/rmgpy/molecule/resonance.py +++ b/rmgpy/molecule/resonance.py @@ -637,14 +637,9 @@ def generate_optimal_aromatic_resonance_structures(mol, features=None): if not features['isCyclic']: return [] + # Copy the molecule so we don't affect the original molecule = mol.copy(deep=True) - # First get all rings in the molecule - rings = molecule.getAllSimpleCyclesOfSize(6) - - # Then determine which ones are aromatic - aromatic_bonds = molecule.getAromaticRings(rings)[1] - # Attempt to rearrange electrons to obtain a structure with the most aromatic rings # Possible rearrangements include aryne resonance and allyl resonance res_list = [generate_aryne_resonance_structures] @@ -658,38 +653,37 @@ def generate_optimal_aromatic_resonance_structures(mol, features=None): _generate_resonance_structures(kekule_list, res_list) - if len(kekule_list) > 1: - # We found additional structures, so we need to evaluate all of them - max_num = 0 - mol_list = [] - - # Iterate through the adjacent resonance structures and keep the structures with the most aromatic rings - for mol0 in kekule_list: - aromatic_bonds = mol0.getAromaticRings()[1] - if len(aromatic_bonds) > max_num: - max_num = len(aromatic_bonds) - mol_list = [(mol0, aromatic_bonds)] - elif len(aromatic_bonds) == max_num: - mol_list.append((mol0, aromatic_bonds)) - else: - # Otherwise, it is not possible to increase the number of aromatic rings by moving electrons, - # so go ahead with the inputted form of the molecule - mol_list = [(molecule, aromatic_bonds)] + # Sort all of the generated structures by number of perceived aromatic rings + mol_dict = {} + for mol0 in kekule_list: + aromatic_bonds = mol0.getAromaticRings()[1] + num_aromatic = len(aromatic_bonds) + mol_dict.setdefault(num_aromatic, []).append((mol0, aromatic_bonds)) + + # List of potential number of aromatic rings, sorted from largest to smallest + arom_options = sorted(mol_dict.keys(), reverse=True) new_mol_list = [] + for num in arom_options: + mol_list = mol_dict[num] + # Generate the aromatic resonance structure(s) + for mol0, aromatic_bonds in mol_list: + # Aromatize the molecule in place + result = generate_aromatic_resonance_structure(mol0, aromatic_bonds, copy=False) + if not result: + # We failed to aromatize this molecule + # This could be due to incorrect aromaticity perception by RDKit + continue - # Generate the aromatic resonance structure(s) - for mol0, aromatic_bonds in mol_list: - # Aromatize the molecule in place - success = generate_aromatic_resonance_structure(mol0, aromatic_bonds, copy=False) - if not success: - continue + for mol1 in new_mol_list: + if mol1.isIsomorphic(mol0): + break + else: + new_mol_list.append(mol0) - for mol1 in new_mol_list: - if mol1.isIsomorphic(mol0): - break - else: - new_mol_list.append(mol0) + if new_mol_list: + # We found the most aromatic resonance structures so there's no need to try smaller numbers + break return new_mol_list From 8289d3c817957e86e25aba9d53ce093b47421eb4 Mon Sep 17 00:00:00 2001 From: Max Liu Date: Tue, 14 May 2019 13:10:50 -0400 Subject: [PATCH 3/4] Copy connectivity values and sorting labels in Molecule.copy Instead of doing it each time we copy a molecule during resonance structure generation. Atom.copy resets connectivity values and does not copy sorting labels, which is reasonable since you might be making a completely different molecule. However, if we're copying a molecule, it makes sense to copy these properties as well. This fixes issues resulting from situations where these values were not copied properly, and also reduces code duplication. --- rmgpy/molecule/molecule.py | 10 +++++- rmgpy/molecule/resonance.py | 63 ------------------------------------- 2 files changed, 9 insertions(+), 64 deletions(-) diff --git a/rmgpy/molecule/molecule.py b/rmgpy/molecule/molecule.py index 821a3574d6..82065da397 100644 --- a/rmgpy/molecule/molecule.py +++ b/rmgpy/molecule/molecule.py @@ -1085,9 +1085,17 @@ def copy(self, deep=False): If `deep` is ``False`` or not specified, a shallow copy is made: the original vertices and edges are used in the new graph. """ - other = cython.declare(Molecule) + cython.declare(g=Graph, other=Molecule, i=int, v1=Vertex, v2=Vertex) g = Graph.copy(self, deep) other = Molecule(g.vertices) + # Copy connectivity values and sorting labels + for i in xrange(len(self.vertices)): + v1 = self.vertices[i] + v2 = other.vertices[i] + v2.connectivity1 = v1.connectivity1 + v2.connectivity2 = v1.connectivity2 + v2.connectivity3 = v1.connectivity3 + v2.sortingLabel = v1.sortingLabel other.multiplicity = self.multiplicity other.reactive = self.reactive return other diff --git a/rmgpy/molecule/resonance.py b/rmgpy/molecule/resonance.py index 1fa1e4399e..dc342357db 100644 --- a/rmgpy/molecule/resonance.py +++ b/rmgpy/molecule/resonance.py @@ -331,15 +331,6 @@ def generate_allyl_delocalization_resonance_structures(mol): bond23.decrementOrder() # Make a copy of structure structure = mol.copy(deep=True) - # Also copy the connectivity values, since they are the same - # for all resonance structures - for index in xrange(len(mol.vertices)): - v1 = mol.vertices[index] - v2 = structure.vertices[index] - v2.connectivity1 = v1.connectivity1 - v2.connectivity2 = v1.connectivity2 - v2.connectivity3 = v1.connectivity3 - v2.sortingLabel = v1.sortingLabel # Restore current structure atom1.incrementRadical() atom3.decrementRadical() @@ -378,15 +369,6 @@ def generate_lone_pair_multiple_bond_resonance_structures(mol): atom3.updateCharge() # Make a copy of structure structure = mol.copy(deep=True) - # Also copy the connectivity values, since they are the same - # for all resonance structures - for index in xrange(len(mol.vertices)): - v1 = mol.vertices[index] - v2 = structure.vertices[index] - v2.connectivity1 = v1.connectivity1 - v2.connectivity2 = v1.connectivity2 - v2.connectivity3 = v1.connectivity3 - v2.sortingLabel = v1.sortingLabel # Restore current structure atom1.incrementLonePairs() atom3.decrementLonePairs() @@ -428,15 +410,6 @@ def generate_adj_lone_pair_radical_resonance_structures(mol): atom2.updateCharge() # Make a copy of structure structure = mol.copy(deep=True) - # Also copy the connectivity values, since they are the same - # for all resonance structures - for index in xrange(len(mol.vertices)): - v1 = mol.vertices[index] - v2 = structure.vertices[index] - v2.connectivity1 = v1.connectivity1 - v2.connectivity2 = v1.connectivity2 - v2.connectivity3 = v1.connectivity3 - v2.sortingLabel = v1.sortingLabel # Restore current structure atom1.incrementRadical() atom1.decrementLonePairs() @@ -479,15 +452,6 @@ def generate_adj_lone_pair_multiple_bond_resonance_structures(mol): atom2.updateCharge() # Make a copy of structure structure = mol.copy(deep=True) - # Also copy the connectivity values, since they are the same - # for all resonance structures - for index in xrange(len(mol.vertices)): - v1 = mol.vertices[index] - v2 = structure.vertices[index] - v2.connectivity1 = v1.connectivity1 - v2.connectivity2 = v1.connectivity2 - v2.connectivity3 = v1.connectivity3 - v2.sortingLabel = v1.sortingLabel # Restore current structure if direction == 1: # The direction the bond order atom1.incrementLonePairs() @@ -541,15 +505,6 @@ def generate_adj_lone_pair_radical_multiple_bond_resonance_structures(mol): atom2.updateCharge() # Make a copy of structure structure = mol.copy(deep=True) - # Also copy the connectivity values, since they are the same - # for all resonance structures - for index in xrange(len(mol.vertices)): - v1 = mol.vertices[index] - v2 = structure.vertices[index] - v2.connectivity1 = v1.connectivity1 - v2.connectivity2 = v1.connectivity2 - v2.connectivity3 = v1.connectivity3 - v2.sortingLabel = v1.sortingLabel # Restore current structure if direction == 1: # The direction the bond order atom1.incrementLonePairs() @@ -593,15 +548,6 @@ def generate_N5dc_radical_resonance_structures(mol): atom3.updateCharge() # Make a copy of structure structure = mol.copy(deep=True) - # Also copy the connectivity values, since they are the same - # for all resonance structures - for index in xrange(len(mol.vertices)): - v1 = mol.vertices[index] - v2 = structure.vertices[index] - v2.connectivity1 = v1.connectivity1 - v2.connectivity2 = v1.connectivity2 - v2.connectivity3 = v1.connectivity3 - v2.sortingLabel = v1.sortingLabel # Restore current structure atom2.incrementRadical() atom2.decrementLonePairs() @@ -818,15 +764,6 @@ def generate_aryne_resonance_structures(mol): bond.setOrderStr(new_orders[i]) # Make a copy of the molecule new_mol = mol.copy(deep=True) - # Also copy the connectivity values, since they are the same - # for all resonance structures - for i in xrange(len(mol.vertices)): - v1 = mol.vertices[i] - v2 = new_mol.vertices[i] - v2.connectivity1 = v1.connectivity1 - v2.connectivity2 = v1.connectivity2 - v2.connectivity3 = v1.connectivity3 - v2.sortingLabel = v1.sortingLabel # Undo the changes to the current molecule for i, bond in enumerate(bond_list): bond.setOrderStr(bond_orders[i]) From a97e70b79d7a0680ec06a913b6305b2ef23efe79 Mon Sep 17 00:00:00 2001 From: Max Liu Date: Tue, 14 May 2019 15:01:04 -0400 Subject: [PATCH 4/4] Add unit test for heteroatom+aromatic resonance structure generation Also update work-in-progress test --- rmgpy/molecule/resonanceTest.py | 34 ++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/rmgpy/molecule/resonanceTest.py b/rmgpy/molecule/resonanceTest.py index 4d96878983..b58492f9b3 100644 --- a/rmgpy/molecule/resonanceTest.py +++ b/rmgpy/molecule/resonanceTest.py @@ -258,13 +258,37 @@ def testAromaticWithLonePairResonance(self): molList = generate_resonance_structures(Molecule(SMILES="c1ccccc1CC=N[O]")) self.assertEqual(len(molList), 4) - @work_in_progress def testAromaticWithNResonance(self): - """Test resonance structure generation for aromatic species with N5ddc <=> N5tc resonance""" - # WIP: currently generate_N5dc_resonance_structures does not apply for aromatic structures + """Test resonance structure generation for aromatic species with lone pair resonance""" molList = generate_resonance_structures(Molecule(SMILES="c1ccccc1CCN=[N+]=[N-]")) - self.assertEqual(len(molList), 4) - # TODO: this test cannot be run because RDKit (which checks for aromaticity) cannot process hyper-valence N + self.assertEqual(len(molList), 2) + + def testAromaticWithONResonance(self): + """Test resonance structure generation for aromatic species with heteroatoms + + This test was specifically designed to recreate RMG-Py issue #1598. + Key conditions: having heteroatoms, starting with aromatic structure, and keep_isomorphic=True + """ + molList = generate_resonance_structures(Molecule().fromAdjacencyList(""" +multiplicity 2 +1 O u0 p2 c0 {4,S} {16,S} +2 O u1 p2 c0 {4,S} +3 O u0 p3 c-1 {4,S} +4 N u0 p0 c+1 {1,S} {2,S} {3,S} {5,S} +5 C u0 p0 c0 {4,S} {6,B} {7,B} +6 C u0 p0 c0 {5,B} {8,B} {11,S} +7 C u0 p0 c0 {5,B} {10,B} {15,S} +8 C u0 p0 c0 {6,B} {9,B} {12,S} +9 C u0 p0 c0 {8,B} {10,B} {13,S} +10 C u0 p0 c0 {7,B} {9,B} {14,S} +11 H u0 p0 c0 {6,S} +12 H u0 p0 c0 {8,S} +13 H u0 p0 c0 {9,S} +14 H u0 p0 c0 {10,S} +15 H u0 p0 c0 {7,S} +16 H u0 p0 c0 {1,S} +"""), keep_isomorphic=True) + self.assertEqual(len(molList), 1) def testNoClarStructures(self): """Test that we can turn off Clar structure generation."""