Don't remove atoms whilst iterating.

Signed-off-by: Egon Willighagen <egonw@users.sourceforge.net>
cdk · Oct 22, 2013 · f03170a · f03170a
1 parent 1a4ac10
commit f03170a
Show file tree

Hide file tree

Showing 2 changed files with 77 additions and 1 deletion.
diff --git a/src/main/org/openscience/cdk/aromaticity/CDKHueckelAromaticityDetector.java b/src/main/org/openscience/cdk/aromaticity/CDKHueckelAromaticityDetector.java
@@ -22,7 +22,9 @@
  */
 package org.openscience.cdk.aromaticity;
 
+import java.util.HashSet;
 import java.util.Iterator;
+import java.util.Set;
 
 import org.openscience.cdk.CDKConstants;
 import org.openscience.cdk.annotations.TestClass;
@@ -79,9 +81,13 @@ public static boolean detectAromaticity(IAtomContainer atomContainer) throws CDK
 			return false;
 		}
 		// disregard all atoms we know that cannot be aromatic anyway
+        Set<IAtom> disregard = new HashSet<IAtom>();
         for (IAtom atom : ringSystems.atoms())
             if (!atomIsPotentiallyAromatic(atom))
-                ringSystems.removeAtomAndConnectedElectronContainers(atom);
+                disregard.add(atom);
+
+        for (IAtom atom : disregard)
+            ringSystems.removeAtomAndConnectedElectronContainers(atom);
 
         // FIXME: should not really mark them here
 		Iterator<IAtom> atoms = ringSystems.atoms().iterator();

diff --git a/src/test/org/openscience/cdk/aromaticity/CDKHueckelAromaticityDetectorTest.java b/src/test/org/openscience/cdk/aromaticity/CDKHueckelAromaticityDetectorTest.java
@@ -41,6 +41,7 @@
 import org.openscience.cdk.interfaces.IAtomContainer;
 import org.openscience.cdk.interfaces.IAtomType;
 import org.openscience.cdk.interfaces.IBond;
+import org.openscience.cdk.interfaces.IChemObjectBuilder;
 import org.openscience.cdk.interfaces.IRing;
 import org.openscience.cdk.interfaces.IRingSet;
 import org.openscience.cdk.io.MDLV2000Reader;
@@ -52,6 +53,8 @@
 import org.openscience.cdk.tools.manipulator.AtomContainerManipulator;
 import org.openscience.cdk.tools.manipulator.RingSetManipulator;
 
+import static org.junit.Assert.assertFalse;
+
 /**
  * @author steinbeck
  * @author egonw
@@ -974,5 +977,72 @@ public void testBug2853035() throws Exception {
         }
     }
 
+    /**
+     * Due to using iterators some Sp3 atoms in the oxaspirodeadiene example
+     * would not be removed and the molcule would incorrectly be found to be
+     * aromatic.
+     * 
+     * @cdk.bug 1313
+     */
+    @Test public void ensureAtomsRemoved() throws Exception {
+        IAtomContainer mol = oxaspirodeadiene();
+        AtomContainerManipulator.percieveAtomTypesAndConfigureAtoms(mol);
+        assertFalse(CDKHueckelAromaticityDetector.detectAromaticity(mol));
+    }
+
+
+    /**
+     * 8-oxaspiro[4.5]deca-6,9-diene
+     * C1CCC2(C1)C=COC=C2
+     * @cdk.inchi InChI=1/C9H12O/c1-2-4-9(3-1)5-7-10-8-6-9/h5-8H,1-4H2
+     */
+    static IAtomContainer oxaspirodeadiene() throws Exception {
+        IChemObjectBuilder builder = DefaultChemObjectBuilder.getInstance();
+        IAtomContainer mol = builder.newInstance(IAtomContainer.class);
+        IAtom a1 = builder.newInstance(IAtom.class,"C");
+        mol.addAtom(a1);
+        IAtom a2 = builder.newInstance(IAtom.class,"C");
+        mol.addAtom(a2);
+        IAtom a3 = builder.newInstance(IAtom.class,"C");
+        mol.addAtom(a3);
+        IAtom a4 = builder.newInstance(IAtom.class,"C");
+        mol.addAtom(a4);
+        IAtom a5 = builder.newInstance(IAtom.class,"C");
+        mol.addAtom(a5);
+        IAtom a6 = builder.newInstance(IAtom.class,"C");
+        mol.addAtom(a6);
+        IAtom a7 = builder.newInstance(IAtom.class,"C");
+        mol.addAtom(a7);
+        IAtom a8 = builder.newInstance(IAtom.class,"O");
+        mol.addAtom(a8);
+        IAtom a9 = builder.newInstance(IAtom.class,"C");
+        mol.addAtom(a9);
+        IAtom a10 = builder.newInstance(IAtom.class,"C");
+        mol.addAtom(a10);
+        IBond b1 = builder.newInstance(IBond.class,a1, a2, IBond.Order.SINGLE);
+        mol.addBond(b1);
+        IBond b2 = builder.newInstance(IBond.class,a2, a3, IBond.Order.SINGLE);
+        mol.addBond(b2);
+        IBond b3 = builder.newInstance(IBond.class,a3, a4, IBond.Order.SINGLE);
+        mol.addBond(b3);
+        IBond b4 = builder.newInstance(IBond.class,a4, a5, IBond.Order.SINGLE);
+        mol.addBond(b4);
+        IBond b5 = builder.newInstance(IBond.class,a1, a5, IBond.Order.SINGLE);
+        mol.addBond(b5);
+        IBond b6 = builder.newInstance(IBond.class,a4, a6, IBond.Order.SINGLE);
+        mol.addBond(b6);
+        IBond b7 = builder.newInstance(IBond.class,a6, a7, IBond.Order.DOUBLE);
+        mol.addBond(b7);
+        IBond b8 = builder.newInstance(IBond.class,a7, a8, IBond.Order.SINGLE);
+        mol.addBond(b8);
+        IBond b9 = builder.newInstance(IBond.class,a8, a9, IBond.Order.SINGLE);
+        mol.addBond(b9);
+        IBond b10 = builder.newInstance(IBond.class,a9, a10, IBond.Order.DOUBLE);
+        mol.addBond(b10);
+        IBond b11 = builder.newInstance(IBond.class,a4, a10, IBond.Order.SINGLE);
+        mol.addBond(b11);
+        return mol;
+    }
+
 }