Fix resonance structure generation and filtration bugs

[mliu49]

Motivation or Problem

Original issue was UndeterminableKineticsError while generating intra_H_migration reaction between C1=C[C]2C=CC3=CC2=C3C1 and [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_structures which would only attempt to aromatize the radical delocalization structures with the largest number of aromatic rings.

The combined effect was as follows:

• For one starting structure, RDKit identified 2 potential aromatic rings, which RMG then determined to be not truly aromatic. Then, it did not attempt to aromatize the valid structures with 1 aromatic ring, leading to no aromatic structures.
• For another starting structure, RDKit only identified 1 potential aromatic ring, which RMG found to be valid, therefore successfully generating one aromatic structure.

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_structures only 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.

[codecov]

Codecov Report

Merging #1600 into master will increase coverage by 0.08%.
The diff coverage is 0%.

@@            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

Impacted Files	Coverage Δ
rmgpy/molecule/resonance.py	0% <0%> (ø)	⬆️
rmgpy/molecule/molecule.py	0% <0%> (ø)	⬆️
rmgpy/data/kinetics/family.py	52.97% <0%> (+0.23%)	⬆️

---

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 baed747...a97e70b. Read the comment docs.

[mliu49]

Additional fix

The 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 Molecule.copy instead of being done in each individual resonance structure method. This ensures that connectivity values and sorting labels are always transferred when copying a molecule.

[alongd]

Looking good! Could you also add a unit test checking the expected resonance structures for cases of combined heteroatom groups and aromatic structures?

[mliu49]

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 keep_isomorphic=True, which prevents running isIsomorphic which would otherwise sort the atoms.

[alongd]

I think it looks good. Would you like to rebase?

[mliu49]

This should be good to go!