Skip to content

Commit f03170a

Browse files
johnmayegonw
johnmay
authored and
egonw
committed
Don't remove atoms whilst iterating.
Signed-off-by: Egon Willighagen <[email protected]>
1 parent 1a4ac10 commit f03170a

2 files changed

Lines changed: 77 additions & 1 deletion

File tree

‎src/main/org/openscience/cdk/aromaticity/CDKHueckelAromaticityDetector.java‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,9 @@
2222
*/
2323
package org.openscience.cdk.aromaticity;
2424

25+
import java.util.HashSet;
2526
import java.util.Iterator;
27+
import java.util.Set;
2628

2729
import org.openscience.cdk.CDKConstants;
2830
import org.openscience.cdk.annotations.TestClass;
@@ -79,9 +81,13 @@ public static boolean detectAromaticity(IAtomContainer atomContainer) throws CDK
7981
return false;
8082
}
8183
// disregard all atoms we know that cannot be aromatic anyway
84+
Set<IAtom> disregard = new HashSet<IAtom>();
8285
for (IAtom atom : ringSystems.atoms())
8386
if (!atomIsPotentiallyAromatic(atom))
84-
ringSystems.removeAtomAndConnectedElectronContainers(atom);
87+
disregard.add(atom);
88+
89+
for (IAtom atom : disregard)
90+
ringSystems.removeAtomAndConnectedElectronContainers(atom);
8591

8692
// FIXME: should not really mark them here
8793
Iterator<IAtom> atoms = ringSystems.atoms().iterator();

‎src/test/org/openscience/cdk/aromaticity/CDKHueckelAromaticityDetectorTest.java‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
import org.openscience.cdk.interfaces.IAtomContainer;
4242
import org.openscience.cdk.interfaces.IAtomType;
4343
import org.openscience.cdk.interfaces.IBond;
44+
import org.openscience.cdk.interfaces.IChemObjectBuilder;
4445
import org.openscience.cdk.interfaces.IRing;
4546
import org.openscience.cdk.interfaces.IRingSet;
4647
import org.openscience.cdk.io.MDLV2000Reader;
@@ -52,6 +53,8 @@
5253
import org.openscience.cdk.tools.manipulator.AtomContainerManipulator;
5354
import org.openscience.cdk.tools.manipulator.RingSetManipulator;
5455

56+
import static org.junit.Assert.assertFalse;
57+
5558
/**
5659
* @author steinbeck
5760
* @author egonw
@@ -974,5 +977,72 @@ public void testBug2853035() throws Exception {
974977
}
975978
}
976979

980+
/**
981+
* Due to using iterators some Sp3 atoms in the oxaspirodeadiene example
982+
* would not be removed and the molcule would incorrectly be found to be
983+
* aromatic.
984+
*
985+
* @cdk.bug 1313
986+
*/
987+
@Test public void ensureAtomsRemoved() throws Exception {
988+
IAtomContainer mol = oxaspirodeadiene();
989+
AtomContainerManipulator.percieveAtomTypesAndConfigureAtoms(mol);
990+
assertFalse(CDKHueckelAromaticityDetector.detectAromaticity(mol));
991+
}
992+
993+
994+
/**
995+
* 8-oxaspiro[4.5]deca-6,9-diene
996+
* C1CCC2(C1)C=COC=C2
997+
* @cdk.inchi InChI=1/C9H12O/c1-2-4-9(3-1)5-7-10-8-6-9/h5-8H,1-4H2
998+
*/
999+
static IAtomContainer oxaspirodeadiene() throws Exception {
1000+
IChemObjectBuilder builder = DefaultChemObjectBuilder.getInstance();
1001+
IAtomContainer mol = builder.newInstance(IAtomContainer.class);
1002+
IAtom a1 = builder.newInstance(IAtom.class,"C");
1003+
mol.addAtom(a1);
1004+
IAtom a2 = builder.newInstance(IAtom.class,"C");
1005+
mol.addAtom(a2);
1006+
IAtom a3 = builder.newInstance(IAtom.class,"C");
1007+
mol.addAtom(a3);
1008+
IAtom a4 = builder.newInstance(IAtom.class,"C");
1009+
mol.addAtom(a4);
1010+
IAtom a5 = builder.newInstance(IAtom.class,"C");
1011+
mol.addAtom(a5);
1012+
IAtom a6 = builder.newInstance(IAtom.class,"C");
1013+
mol.addAtom(a6);
1014+
IAtom a7 = builder.newInstance(IAtom.class,"C");
1015+
mol.addAtom(a7);
1016+
IAtom a8 = builder.newInstance(IAtom.class,"O");
1017+
mol.addAtom(a8);
1018+
IAtom a9 = builder.newInstance(IAtom.class,"C");
1019+
mol.addAtom(a9);
1020+
IAtom a10 = builder.newInstance(IAtom.class,"C");
1021+
mol.addAtom(a10);
1022+
IBond b1 = builder.newInstance(IBond.class,a1, a2, IBond.Order.SINGLE);
1023+
mol.addBond(b1);
1024+
IBond b2 = builder.newInstance(IBond.class,a2, a3, IBond.Order.SINGLE);
1025+
mol.addBond(b2);
1026+
IBond b3 = builder.newInstance(IBond.class,a3, a4, IBond.Order.SINGLE);
1027+
mol.addBond(b3);
1028+
IBond b4 = builder.newInstance(IBond.class,a4, a5, IBond.Order.SINGLE);
1029+
mol.addBond(b4);
1030+
IBond b5 = builder.newInstance(IBond.class,a1, a5, IBond.Order.SINGLE);
1031+
mol.addBond(b5);
1032+
IBond b6 = builder.newInstance(IBond.class,a4, a6, IBond.Order.SINGLE);
1033+
mol.addBond(b6);
1034+
IBond b7 = builder.newInstance(IBond.class,a6, a7, IBond.Order.DOUBLE);
1035+
mol.addBond(b7);
1036+
IBond b8 = builder.newInstance(IBond.class,a7, a8, IBond.Order.SINGLE);
1037+
mol.addBond(b8);
1038+
IBond b9 = builder.newInstance(IBond.class,a8, a9, IBond.Order.SINGLE);
1039+
mol.addBond(b9);
1040+
IBond b10 = builder.newInstance(IBond.class,a9, a10, IBond.Order.DOUBLE);
1041+
mol.addBond(b10);
1042+
IBond b11 = builder.newInstance(IBond.class,a4, a10, IBond.Order.SINGLE);
1043+
mol.addBond(b11);
1044+
return mol;
1045+
}
1046+
9771047
}
9781048

0 commit comments

Comments
 (0)