Fix resonance structure generation and filtration bugs #1600
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1600 +/- ##
==========================================
+ Coverage 41.58% 41.67% +0.08%
==========================================
Files 176 176
Lines 29147 29102 -45
Branches 5995 5988 -7
==========================================
+ Hits 12122 12127 +5
+ Misses 16187 16138 -49
+ Partials 838 837 -1
Continue to review full report at Codecov.
|
Additional fixThe cause of #1578 and #1598 was identified to be lack of valid atom sorting labels. Octet and charge filtration both rely on sorting labels to identify atoms. In this case, Kekule structure generation was not copying connectivity values and sorting labels when copying the molecule. The resulting molecule had sorting labels of -1 for all atoms, leading to issues during filtration. The chosen solution was to move connectivity value and sorting label copying to |
mliu49
added
the
Status: Ready for Review
mliu49
changed the title
|
Looking good! Could you also add a unit test checking the expected resonance structures for cases of combined heteroatom groups and aromatic structures? |
|
I added a unit test. It took me a while to recreate the error message in a unit test, but I figured out that the key was having |
|
I think it looks good. Would you like to rebase? |
Depending on order of atoms in adjlist, different resonance structures are generated. Root cause seems to be difference in aromaticity perception by RDKit.
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.
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.
Also update work-in-progress test
|
This should be good to go! |
Motivation or Problem
Original issue was
UndeterminableKineticsErrorwhile generatingintra_H_migrationreaction betweenC1=C[C]2C=CC3=CC2=C3C1and[C]1=CC2=CC3=C2CC=CC13.Cause was inconsistent resonance structure generation resulting from minor differences in molecule objects, which led to the inability to identify the correct reverse reaction.
The cause of the resonance structure inconsistency was traced back to an inconsistency in perceived aromatic rings as determined by
Molecule.getAromaticRings(), which relies on RDKit.This was compounded by a bug in
generate_optimal_aromatic_resonance_structureswhich would only attempt to aromatize the radical delocalization structures with the largest number of aromatic rings.The combined effect was as follows:
Description of Changes
This does not fix the root cause, which is inconsistent aromaticity perception. It does fix the second issue where
generate_optimal_aromatic_resonance_structuresonly tries to aromatize the structures with the most aromatic rings.This modifies the algorithm to instead try all of the delocalized structures, starting with the ones with the most aromatic rings. If that fails, it will move to the structures with the second most aromatic rings, and so on.
This also adds a failing unit test with the bug that was discovered.
With this change, RMG can successfully generate the aforementioned reaction.
Testing
Try generating reactions for the molecules mentioned above.