From 3be2367b36da324ae45ce61a2d503cd024a63a19 Mon Sep 17 00:00:00 2001 From: Egon Willighagen Date: Sat, 27 Mar 2010 20:10:44 +0100 Subject: [PATCH 1/8] Two more tests for the issue: atom typing works fine; aromaticity detection fails: one ring is detected as aromatic (that with two nitrogesn), so that it does not consider the double ring, marking the other ring as non-aromatic Signed-off-by: Rajarshi Guha --- .../CDKHueckelAromaticityDetectorTest.java | 11 +++++++++++ .../cdk/atomtype/CDKAtomTypeMatcherSMILESTest.java | 14 ++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/src/test/org/openscience/cdk/aromaticity/CDKHueckelAromaticityDetectorTest.java b/src/test/org/openscience/cdk/aromaticity/CDKHueckelAromaticityDetectorTest.java index 3210b7c94dd..373f5a6b201 100644 --- a/src/test/org/openscience/cdk/aromaticity/CDKHueckelAromaticityDetectorTest.java +++ b/src/test/org/openscience/cdk/aromaticity/CDKHueckelAromaticityDetectorTest.java @@ -740,6 +740,17 @@ public void testPolyCyclicSystem() throws Exception { Assert.assertTrue(isAromatic); } + /** + * @cdk.bug 2976054 + */ + @Test public void testAnotherNitrogen_SP2() throws Exception { + SmilesParser sp = new SmilesParser(DefaultChemObjectBuilder.getInstance()); + IMolecule mol = sp.parseSmiles("c1cnc2s[cH][cH]n12"); + AtomContainerManipulator.percieveAtomTypesAndConfigureAtoms(mol); + Assert.assertFalse(CDKHueckelAromaticityDetector.detectAromaticity(mol)); + for (IAtom atom : mol.atoms()) + Assert.assertTrue(atom.getFlag(CDKConstants.ISAROMATIC)); + } } diff --git a/src/test/org/openscience/cdk/atomtype/CDKAtomTypeMatcherSMILESTest.java b/src/test/org/openscience/cdk/atomtype/CDKAtomTypeMatcherSMILESTest.java index 6af358129c9..90a5b40421f 100644 --- a/src/test/org/openscience/cdk/atomtype/CDKAtomTypeMatcherSMILESTest.java +++ b/src/test/org/openscience/cdk/atomtype/CDKAtomTypeMatcherSMILESTest.java @@ -94,6 +94,20 @@ public class CDKAtomTypeMatcherSMILESTest extends AbstractCDKAtomTypeTest { Assert.assertNotNull(type.getAtomTypeName()); } } + + /** + * @cdk.bug 2976054 + */ + @Test public void testAnotherNitrogen_SP2() throws Exception { + String smiles1 = "c1cnc2s[cH][cH]n12"; + IMolecule mol1 = smilesParser.parseSmiles(smiles1); + + Assert.assertEquals(8, mol1.getAtomCount()); + IAtomType[] types1 = atomTypeMatcher.findMatchingAtomType(mol1); + for (IAtomType type : types1) { + Assert.assertNotNull(type.getAtomTypeName()); + } + } @Test public void testPlatinum4() throws Exception { String smiles1 = "Cl[Pt]1(Cl)(Cl)(Cl)NC2CCCCC2N1"; From 068fb3b9b2965668d131f527257c56f65fdd65e6 Mon Sep 17 00:00:00 2001 From: Egon Willighagen Date: Tue, 6 Apr 2010 15:30:40 +0200 Subject: [PATCH 2/8] Moved test from the specific class to the abstract tests, as the behavior should be the same for NNMoleculeSet and DebugMoleculeSet too Signed-off-by: Rajarshi Guha --- .../org/openscience/cdk/MoleculeSetTest.java | 18 +----------------- .../interfaces/AbstractMoleculeSetTest.java | 13 ++++++++++++- 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/src/test/org/openscience/cdk/MoleculeSetTest.java b/src/test/org/openscience/cdk/MoleculeSetTest.java index 18e5045eadb..004c8a2e2bb 100644 --- a/src/test/org/openscience/cdk/MoleculeSetTest.java +++ b/src/test/org/openscience/cdk/MoleculeSetTest.java @@ -20,14 +20,9 @@ */ package org.openscience.cdk; -import junit.framework.Assert; - import org.junit.BeforeClass; -import org.junit.Test; import org.openscience.cdk.interfaces.AbstractMoleculeSetTest; import org.openscience.cdk.interfaces.IChemObject; -import org.openscience.cdk.interfaces.IMolecule; -import org.openscience.cdk.interfaces.IMoleculeSet; import org.openscience.cdk.interfaces.ITestObjectBuilder; /** @@ -46,16 +41,5 @@ public IChemObject newTestObject() { } }); } - - @Test public void testClone() throws CloneNotSupportedException{ - IMoleculeSet moleculeSet = DefaultChemObjectBuilder.getInstance() - .newMoleculeSet(); - IMolecule mol = moleculeSet.getBuilder().newMolecule(); - moleculeSet.addAtomContainer(mol); - //we test that the molecule added is actually in the moleculeSet - Assert.assertSame(mol, moleculeSet.getAtomContainer(0)); - moleculeSet.clone(); - //after the clone, the molecule added should still be in the moleculeSet - Assert.assertSame(mol, moleculeSet.getAtomContainer(0)); - } + } diff --git a/src/test/org/openscience/cdk/interfaces/AbstractMoleculeSetTest.java b/src/test/org/openscience/cdk/interfaces/AbstractMoleculeSetTest.java index 6e41481004a..fcf76c6deb4 100644 --- a/src/test/org/openscience/cdk/interfaces/AbstractMoleculeSetTest.java +++ b/src/test/org/openscience/cdk/interfaces/AbstractMoleculeSetTest.java @@ -170,7 +170,18 @@ public abstract class AbstractMoleculeSetTest extends AbstractAtomContainerSetTe Assert.assertTrue(clone instanceof IMoleculeSet); Assert.assertNotSame(som, clone); } - + + @Test public void testCloneButKeepOriginalIntact() throws CloneNotSupportedException{ + IMoleculeSet moleculeSet = (IMoleculeSet)newChemObject(); + IMolecule mol = moleculeSet.getBuilder().newMolecule(); + moleculeSet.addAtomContainer(mol); + //we test that the molecule added is actually in the moleculeSet + Assert.assertSame(mol, moleculeSet.getAtomContainer(0)); + moleculeSet.clone(); + //after the clone, the molecule added should still be in the moleculeSet + Assert.assertSame(mol, moleculeSet.getAtomContainer(0)); + } + @Test public void testCloneDuplication() throws Exception { IMoleculeSet moleculeSet = (IMoleculeSet)newChemObject(); moleculeSet.addMolecule(moleculeSet.getBuilder().newMolecule()); From 4e5d6a120f1e92ab70f93c3e4fbc0cfc92495a12 Mon Sep 17 00:00:00 2001 From: Egon Willighagen Date: Tue, 6 Apr 2010 15:46:07 +0200 Subject: [PATCH 3/8] Apparently the super.clone() does not clone the pointer to the IAtomContainer[], causing a clone() followed by changing containers in the clone to overwrite the original IAtomContainer[]. Fixed by creating a new array. Signed-off-by: Rajarshi Guha --- src/main/org/openscience/cdk/AtomContainerSet.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/main/org/openscience/cdk/AtomContainerSet.java b/src/main/org/openscience/cdk/AtomContainerSet.java index 37554fdcb89..c653dade56a 100644 --- a/src/main/org/openscience/cdk/AtomContainerSet.java +++ b/src/main/org/openscience/cdk/AtomContainerSet.java @@ -365,9 +365,11 @@ public String toString() { */ public Object clone() throws CloneNotSupportedException { AtomContainerSet clone = (AtomContainerSet)super.clone(); + clone.atomContainers = new AtomContainer[atomContainerCount]; + clone.atomContainerCount = 0; for (int i = 0; i < atomContainerCount; i++) { - clone.replaceAtomContainer(i, (IAtomContainer)atomContainers[i].clone()); - clone.setMultiplier(i, getMultiplier(i)); + clone.addAtomContainer((IAtomContainer)atomContainers[i].clone()); + clone.setMultiplier(i, getMultiplier(i)); } return clone; } From 216c1600b937b846db56e6c48fbc9a8db65a2c59 Mon Sep 17 00:00:00 2001 From: Egon Willighagen Date: Tue, 6 Apr 2010 15:47:37 +0200 Subject: [PATCH 4/8] Removed duplication of cloning. Signed-off-by: Rajarshi Guha --- src/main/org/openscience/cdk/MoleculeSet.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/main/org/openscience/cdk/MoleculeSet.java b/src/main/org/openscience/cdk/MoleculeSet.java index 80032cd5206..a15e443824b 100644 --- a/src/main/org/openscience/cdk/MoleculeSet.java +++ b/src/main/org/openscience/cdk/MoleculeSet.java @@ -171,11 +171,7 @@ public int getMoleculeCount() { *@return the cloned object */ public Object clone() throws CloneNotSupportedException { - MoleculeSet clone = (MoleculeSet)super.clone(); - for (int i = 0; i < atomContainerCount; i++) { - clone.replaceAtomContainer(i, (IAtomContainer)atomContainers[i].clone()); - } - return (Object) clone; + return (MoleculeSet)super.clone(); } public String toString() { From 3c1b07e26f1bfdb1447782c6f1ff98f5f9e42539 Mon Sep 17 00:00:00 2001 From: Egon Willighagen Date: Tue, 6 Apr 2010 15:47:39 +0200 Subject: [PATCH 5/8] Unit test that the IAtom[] array is properly cloned, and overwriting entries in the clone does not overwrite entries on the original Signed-off-by: Rajarshi Guha --- .../cdk/interfaces/AbstractAtomContainerTest.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/test/org/openscience/cdk/interfaces/AbstractAtomContainerTest.java b/src/test/org/openscience/cdk/interfaces/AbstractAtomContainerTest.java index c57c08e1e68..541b686d657 100644 --- a/src/test/org/openscience/cdk/interfaces/AbstractAtomContainerTest.java +++ b/src/test/org/openscience/cdk/interfaces/AbstractAtomContainerTest.java @@ -75,7 +75,18 @@ public abstract class AbstractAtomContainerTest extends AbstractChemObjectTest { } } } - + + @Test public void testCloneButKeepOriginalsIntact() throws Exception { + IMolecule molecule = (IMolecule)newChemObject(); + IAtom atom = molecule.getBuilder().newAtom(); + molecule.addAtom(atom); + Assert.assertEquals(atom, molecule.getAtom(0)); + Object clone = molecule.clone(); + Assert.assertNotSame(molecule, clone); + // after the cloning the IAtom on the original IMolecule should be unchanged + Assert.assertEquals(atom, molecule.getAtom(0)); + } + @Test public void testClone_IAtom2() throws Exception { IAtomContainer molecule = (IAtomContainer)newChemObject(); IAtom carbon = molecule.getBuilder().newAtom("C"); From 38d5f8d1915147db9a352299e5f93430634d1ca2 Mon Sep 17 00:00:00 2001 From: Egon Willighagen Date: Tue, 6 Apr 2010 16:53:21 +0200 Subject: [PATCH 6/8] Added unit test to see of arrays are properly cloned, and that array entries of the original are not overwritten Signed-off-by: Rajarshi Guha --- .../interfaces/AbstractAtomContainerTest.java | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/test/org/openscience/cdk/interfaces/AbstractAtomContainerTest.java b/src/test/org/openscience/cdk/interfaces/AbstractAtomContainerTest.java index 541b686d657..970a8f4542b 100644 --- a/src/test/org/openscience/cdk/interfaces/AbstractAtomContainerTest.java +++ b/src/test/org/openscience/cdk/interfaces/AbstractAtomContainerTest.java @@ -87,6 +87,51 @@ public abstract class AbstractAtomContainerTest extends AbstractChemObjectTest { Assert.assertEquals(atom, molecule.getAtom(0)); } + @Test public void testCloneButKeepOriginalsIntact_IBond() throws Exception { + IAtomContainer molecule = (IAtomContainer)newChemObject(); + molecule.addAtom(molecule.getBuilder().newAtom()); + molecule.addAtom(molecule.getBuilder().newAtom()); + IBond bond = molecule.getBuilder().newBond( + molecule.getAtom(0), + molecule.getAtom(1), + IBond.Order.SINGLE + ); + molecule.addBond(bond); + Assert.assertEquals(bond, molecule.getBond(0)); + Object clone = molecule.clone(); + Assert.assertNotSame(molecule, clone); + // after the cloning the IBond on the original IMolecule should be unchanged + Assert.assertEquals(bond, molecule.getBond(0)); + } + + @Test public void testCloneButKeepOriginalsIntact_ILonePair() throws Exception { + IAtomContainer molecule = (IAtomContainer)newChemObject(); + molecule.addAtom(molecule.getBuilder().newAtom()); + ILonePair lonePair = molecule.getBuilder().newLonePair( + molecule.getAtom(0) + ); + molecule.addLonePair(lonePair); + Assert.assertEquals(lonePair, molecule.getLonePair(0)); + Object clone = molecule.clone(); + Assert.assertNotSame(molecule, clone); + // after the cloning the ILonePair on the original IMolecule should be unchanged + Assert.assertEquals(lonePair, molecule.getLonePair(0)); + } + + @Test public void testCloneButKeepOriginalsIntact_ISingleElectron() throws Exception { + IAtomContainer molecule = (IAtomContainer)newChemObject(); + molecule.addAtom(molecule.getBuilder().newAtom()); + ISingleElectron singleElectron = molecule.getBuilder().newSingleElectron( + molecule.getAtom(0) + ); + molecule.addSingleElectron(singleElectron); + Assert.assertEquals(singleElectron, molecule.getSingleElectron(0)); + Object clone = molecule.clone(); + Assert.assertNotSame(molecule, clone); + // after the cloning the ISingleElectron on the original IMolecule should be unchanged + Assert.assertEquals(singleElectron, molecule.getSingleElectron(0)); + } + @Test public void testClone_IAtom2() throws Exception { IAtomContainer molecule = (IAtomContainer)newChemObject(); IAtom carbon = molecule.getBuilder().newAtom("C"); From f95c6324a8d3f81c66a2e2eb1c26899feb642e33 Mon Sep 17 00:00:00 2001 From: Rajarshi Guha Date: Sat, 10 Apr 2010 01:17:04 -0400 Subject: [PATCH 7/8] Updated HIN reader to fix bug 2984581 --- .../org/openscience/cdk/io/HINReader.java | 35 +++++++++++-------- src/test/data/hin/bug2984581.hin | 31 ++++++++++++++++ .../org/openscience/cdk/io/HINReaderTest.java | 21 ++++++++--- 3 files changed, 69 insertions(+), 18 deletions(-) create mode 100644 src/test/data/hin/bug2984581.hin diff --git a/src/main/org/openscience/cdk/io/HINReader.java b/src/main/org/openscience/cdk/io/HINReader.java index f5a6d06d0dd..3d6f549f46c 100644 --- a/src/main/org/openscience/cdk/io/HINReader.java +++ b/src/main/org/openscience/cdk/io/HINReader.java @@ -20,18 +20,6 @@ */ package org.openscience.cdk.io; -import java.io.BufferedReader; -import java.io.IOException; -import java.io.InputStream; -import java.io.InputStreamReader; -import java.io.Reader; -import java.io.StringReader; -import java.util.ArrayList; -import java.util.List; -import java.util.StringTokenizer; - -import javax.vecmath.Point3d; - import org.openscience.cdk.CDKConstants; import org.openscience.cdk.annotations.TestClass; import org.openscience.cdk.annotations.TestMethod; @@ -48,6 +36,17 @@ import org.openscience.cdk.io.formats.HINFormat; import org.openscience.cdk.io.formats.IResourceFormat; +import javax.vecmath.Point3d; +import java.io.BufferedReader; +import java.io.IOException; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.io.Reader; +import java.io.StringReader; +import java.util.ArrayList; +import java.util.List; +import java.util.StringTokenizer; + /** * Reads an object from HIN formated input. * @@ -197,7 +196,7 @@ private IChemFile readChemFile(IChemFile file) { // read data for current molecule int atomSerial = 0; while (true) { - if (line.indexOf("endmol ") >= 0) { + if (line == null || line.indexOf("endmol ") >= 0) { break; } if (line.indexOf(';') == 0) continue; // comment line @@ -257,7 +256,15 @@ private IChemFile readChemFile(IChemFile file) { m.addBond(file.getBuilder().newBond(s, e, bo)); } setOfMolecules.addMolecule(m); - line = input.readLine(); // read in the 'mol N' + + // we may not get a 'mol N' immediately since + // the aromaticring keyword might be present + // and doesn't seem to be located within the molecule + // block + while (true) { + line = input.readLine(); + if (line == null || line.indexOf("mol ") == 0) break; + } } // got all the molecule in the HIN file (hopefully!) diff --git a/src/test/data/hin/bug2984581.hin b/src/test/data/hin/bug2984581.hin new file mode 100644 index 00000000000..2c694644f2a --- /dev/null +++ b/src/test/data/hin/bug2984581.hin @@ -0,0 +1,31 @@ +forcefield mm+ +sys 0 0 1 +view 40 0.1471 55 15 0.5586844 -0.7573253 0.3381274 0.8112321 0.583803 -0.03280994 -0.172552 0.2926302 0.9405283 -1.5498 -0.69331 -55.277 +seed 0 +mol 1 +atom 1 - C CA - -0.1921654 0.5444375 -0.480369 0.1121711 3 3 a 2 a 14 s +atom 2 - C CA - -0.07865381 1.48883 -1.507176 0.1121711 3 5 a 1 a 15 s +atom 3 - C CA - 0.1260283 0.9869497 0.8709679 0.1121711 3 1 a 4 a 7 s +atom 4 - C CA - -0.1945596 2.357387 1.141245 0.1121711 3 3 a 6 a 20 s +atom 5 - C CA - -0.1604924 2.854465 -1.227169 0.1121711 3 2 a 6 a 21 s +atom 6 - C CA - -0.08668089 3.281385 0.09564244 0.1121711 3 5 a 4 a 16 s +atom 7 - O O2 - -0.1584311 -0.06161446 1.767233 0.1121711 2 3 s 8 s +atom 8 - C CA - 0.09127665 -0.009773561 3.152164 0.1121711 3 13 a 7 s 9 a +atom 9 - C CA - -0.1865864 1.073688 4.026796 0.1121711 3 10 a 22 s 8 a +atom 10 - C CA - -0.1031656 0.8394372 5.407645 0.1121711 3 9 a 11 a 17 s +atom 11 - C CA - -0.153739 -0.452265 5.91707 0.1121711 3 10 a 12 a 18 s +atom 12 - C CA - -0.1025772 -1.539669 5.037201 0.1121711 3 11 a 13 a 19 s +atom 13 - C CA - -0.1454592 -1.331929 3.667263 0.1121711 3 12 a 8 a 23 s +atom 14 - Br BR - 0.07038784 -1.265702 -0.9827618 0.1121711 1 1 s +atom 15 - H H - 0.149111 1.15467 -2.557919 0.1121711 1 2 s +atom 16 - H H - 0.1373875 4.357067 0.3297046 0.1121711 1 6 s +atom 17 - H H - 0.1337775 1.701587 6.091895 0.1121711 1 10 s +atom 18 - H H - 0.1358719 -0.6226702 7.002957 0.1121711 1 11 s +atom 19 - H H - 0.1372966 -2.566783 5.432249 0.1121711 1 12 s +atom 20 - H H - 0.145665 2.75555 2.159567 0.1121711 1 4 s +atom 21 - H H - 0.1408581 3.583701 -2.04989 0.1121711 1 5 s +atom 22 - H H - 0.1395921 2.117928 3.702678 0.1121711 1 9 s +atom 23 - H H - 0.1552575 -2.176271 2.961693 0.1121711 1 13 s +endmol 1 +aromaticring 6 1 1 1 3 1 4 1 6 1 5 1 2 +aromaticring 6 1 8 1 13 1 12 1 11 1 10 1 9 diff --git a/src/test/org/openscience/cdk/io/HINReaderTest.java b/src/test/org/openscience/cdk/io/HINReaderTest.java index 82b8d10337c..1921e3c0f61 100644 --- a/src/test/org/openscience/cdk/io/HINReaderTest.java +++ b/src/test/org/openscience/cdk/io/HINReaderTest.java @@ -27,20 +27,19 @@ * */ package org.openscience.cdk.io; -import java.io.InputStream; -import java.util.List; - import org.junit.Assert; import org.junit.BeforeClass; import org.junit.Test; import org.openscience.cdk.ChemFile; import org.openscience.cdk.ChemObject; -import org.openscience.cdk.exception.CDKException; import org.openscience.cdk.interfaces.IAtomContainer; import org.openscience.cdk.interfaces.IChemFile; import org.openscience.cdk.tools.LoggingTool; import org.openscience.cdk.tools.manipulator.ChemFileManipulator; +import java.io.InputStream; +import java.util.List; + /** * TestCase for the reading HIN mol files using one test file. * @@ -139,4 +138,18 @@ public class HINReaderTest extends SimpleChemObjectReaderTest { Assert.assertEquals(57, ac.getAtomCount()); Assert.assertEquals(59, ac.getBondCount()); } + + /** + * @cdk.bug 2984581 + * @throws Exception + */ + @Test public void testAromaticRingsLine() throws Exception { + String filename = "data/hin/bug2984581.hin"; + InputStream ins = this.getClass().getClassLoader().getResourceAsStream(filename); + ISimpleChemObjectReader reader = new HINReader(ins); + IChemFile content = (IChemFile) reader.read(new ChemFile()); + List cList = ChemFileManipulator.getAllAtomContainers(content); + Assert.assertEquals(1, cList.size()); + IAtomContainer ac = cList.get(0); + } } From 7b9d84e44a407af71edafb8c650048981041c3bc Mon Sep 17 00:00:00 2001 From: Rajarshi Guha Date: Sat, 10 Apr 2010 08:56:00 -0400 Subject: [PATCH 8/8] converted uses of indexOf to startsWith/contains --- src/main/org/openscience/cdk/io/HINReader.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/main/org/openscience/cdk/io/HINReader.java b/src/main/org/openscience/cdk/io/HINReader.java index 3d6f549f46c..a4b65b0086d 100644 --- a/src/main/org/openscience/cdk/io/HINReader.java +++ b/src/main/org/openscience/cdk/io/HINReader.java @@ -167,7 +167,7 @@ private IChemFile readChemFile(IChemFile file) { // read in header info while (true) { line = input.readLine(); - if (line.indexOf("mol ") == 0) { + if (line.startsWith("mol")) { info = getMolName(line); break; } @@ -178,9 +178,9 @@ private IChemFile readChemFile(IChemFile file) { line = input.readLine(); while(true) { if (line == null) break; // end of file - if (line.indexOf(';') == 0) continue; // comment line + if (line.startsWith(";")) continue; // comment line - if (line.indexOf("mol ") == 0) { + if (line.startsWith("mol")) { info = getMolName(line); line = input.readLine(); } @@ -196,10 +196,10 @@ private IChemFile readChemFile(IChemFile file) { // read data for current molecule int atomSerial = 0; while (true) { - if (line == null || line.indexOf("endmol ") >= 0) { + if (line == null || line.contains("endmol")) { break; } - if (line.indexOf(';') == 0) continue; // comment line + if (line.startsWith(";")) continue; // comment line tokenizer = new StringTokenizer(line, " "); @@ -263,7 +263,7 @@ private IChemFile readChemFile(IChemFile file) { // block while (true) { line = input.readLine(); - if (line == null || line.indexOf("mol ") == 0) break; + if (line == null || line.startsWith("mol")) break; } }