Merge pull request #208 from cdk/patch/api-fixup

Looks fine.
cdk · Jul 23, 2016 · 397a0c5 · 397a0c5
2 parents 2ade769 + 2f2ccc4
commit 397a0c5
Show file tree

Hide file tree

Showing 13 changed files with 84 additions and 25 deletions.
diff --git a/...tandard/src/main/java/org/openscience/cdk/tools/manipulator/AtomContainerManipulator.java b/...tandard/src/main/java/org/openscience/cdk/tools/manipulator/AtomContainerManipulator.java
@@ -146,6 +146,8 @@ public static boolean replaceAtomByAtom(final IAtomContainer container, final IA
                 container.setAtom(i, newAtom);
                 atomremap.put(oldAtom, newAtom);
             } else {
+                if (atom == newAtom)
+                    throw new IllegalArgumentException("Cannot replace atom with one from the same molecule.");
                 atomremap.put(atom, atom);
             }
         }

diff --git a/base/test/src/test/java/org/openscience/cdk/CDKTestCase.java b/base/test/src/test/java/org/openscience/cdk/CDKTestCase.java
@@ -192,9 +192,11 @@ protected void addImplicitHydrogens(IAtomContainer container) throws Exception {
      *
      * @param container the atom container to check
      */
-    protected void assertAllSingleAndAromatic(IAtomContainer container) throws Exception {
-        for (Iterator<IBond> bonds = container.bonds().iterator(); bonds.hasNext();)
-            Assert.assertEquals(IBond.Order.SINGLE, bonds.next().getOrder());
+    protected void assertAllSingleOrAromatic(IAtomContainer container) throws Exception {
+        for (IBond bond : container.bonds()) {
+            if (!bond.isAromatic())
+                Assert.assertEquals(IBond.Order.SINGLE, bond.getOrder());
+        }
 
         for (IAtom atom : container.atoms()) {
             if (atom.getSymbol().equals("H"))

diff --git a/storage/smiles/src/main/java/org/openscience/cdk/smiles/BeamToCDK.java b/storage/smiles/src/main/java/org/openscience/cdk/smiles/BeamToCDK.java
@@ -355,12 +355,15 @@ IBond toCDKBond(Edge edge, IAtom[] atoms) {
                 bond.setIsAromatic(true);
                 atoms[u].setIsAromatic(true);
                 atoms[v].setIsAromatic(true);
+                break;
             case IMPLICIT:
                 if (atoms[u].isAromatic() && atoms[v].isAromatic()) {
                     bond.setIsAromatic(true);
+                    bond.setOrder(IBond.Order.UNSET);
                     atoms[u].setIsAromatic(true);
                     atoms[v].setIsAromatic(true);
                 }
+                break;
         }
 
         return bond;

diff --git a/storage/smiles/src/main/java/org/openscience/cdk/smiles/CDKToBeam.java b/storage/smiles/src/main/java/org/openscience/cdk/smiles/CDKToBeam.java
@@ -26,6 +26,7 @@
 
 import com.google.common.collect.Maps;
 
+import org.openscience.cdk.CDK;
 import org.openscience.cdk.CDKConstants;
 import org.openscience.cdk.config.Isotopes;
 import org.openscience.cdk.config.IsotopeFactory;
@@ -267,6 +268,8 @@ private Bond toBeamEdgeLabel(IBond b) throws CDKException {
             case QUADRUPLE:
                 return Bond.QUADRUPLE;
             default:
+                if (!this.aromatic && b.getFlag(CDKConstants.ISAROMATIC))
+                    throw new CDKException("Cannot write Kekulé SMILES output due to aromatic bond with unset bond order - molecule should be Kekulized");
                 throw new CDKException("Unsupported bond order: " + order);
         }
     }

diff --git a/storage/smiles/src/main/java/org/openscience/cdk/smiles/DeduceBondSystemTool.java b/storage/smiles/src/main/java/org/openscience/cdk/smiles/DeduceBondSystemTool.java
@@ -123,6 +123,13 @@ public boolean isOK(IAtomContainer m) throws CDKException {
      * @throws CDKException if something went wrong.
      */
     public IAtomContainer fixAromaticBondOrders(IAtomContainer atomContainer) throws CDKException {
+
+        // preset all bond orders to single
+        for (IBond bond : atomContainer.bonds()) {
+            if (bond.isAromatic() && bond.getOrder() == IBond.Order.UNSET)
+                bond.setOrder(IBond.Order.SINGLE);
+        }
+
         // OK, we take advantage here from the fact that this class does not take
         // into account rings larger than 7 atoms. See fixAromaticBondOrders().
         IRingSet rs = allRingsFinder.findAllRings(atomContainer, 7);

diff --git a/storage/smiles/src/main/java/org/openscience/cdk/smiles/FixBondOrdersTool.java b/storage/smiles/src/main/java/org/openscience/cdk/smiles/FixBondOrdersTool.java
@@ -65,7 +65,9 @@
  * @author Lucy Entwistle
  * @cdk.module smiles
  * @cdk.githash
+ * @deprecated use {@link org.openscience.cdk.aromaticity.Kekulization}
  */
+@Deprecated
 public class FixBondOrdersTool {
 
     private boolean interrupted;

diff --git a/storage/smiles/src/main/java/org/openscience/cdk/smiles/SmilesGenerator.java b/storage/smiles/src/main/java/org/openscience/cdk/smiles/SmilesGenerator.java
@@ -176,7 +176,10 @@ public final class SmilesGenerator {
     /**
      * Create the generic SMILES generator.
      * @see #generic()
+     * @deprecated some consideration is needed on what SMILES is required e.g. SmilesGenerator.unique()
+     *             vs SmilesGenerator.isomeric();
      */
+    @Deprecated
     public SmilesGenerator() {
         this(false, false, false, false);
     }

diff --git a/storage/smiles/src/test/java/org/openscience/cdk/smiles/BeamToCDKTest.java b/storage/smiles/src/test/java/org/openscience/cdk/smiles/BeamToCDKTest.java
@@ -196,7 +196,7 @@ public void benzene() throws IOException {
             assertThat(a.getImplicitHydrogenCount(), is(1));
         }
         for (IBond b : ac.bonds()) {
-            assertThat(b.getOrder(), is(IBond.Order.SINGLE));
+            assertThat(b.getOrder(), is(IBond.Order.UNSET));
             assertTrue(b.getFlag(CDKConstants.ISAROMATIC));
         }
     }
@@ -253,7 +253,7 @@ public void imidazole() throws IOException {
         }
 
         for (IBond b : ac.bonds()) {
-            assertThat(b.getOrder(), is(IBond.Order.SINGLE));
+            assertThat(b.getOrder(), is(IBond.Order.UNSET));
             assertTrue(b.getFlag(CDKConstants.ISAROMATIC));
         }
     }

diff --git a/storage/smiles/src/test/java/org/openscience/cdk/smiles/DeduceBondSystemToolTest.java b/storage/smiles/src/test/java/org/openscience/cdk/smiles/DeduceBondSystemToolTest.java
@@ -75,6 +75,7 @@ public void testPyrrole() throws Exception {
         SmilesParser smilesParser = new SmilesParser(DefaultChemObjectBuilder.getInstance());
         smilesParser.kekulise(false);
         IAtomContainer molecule = smilesParser.parseSmiles(smiles);
+        AtomContainerManipulator.setSingleOrDoubleFlags(molecule);
         AtomContainerManipulator.percieveAtomTypesAndConfigureAtoms(molecule);
 
         molecule = dbst.fixAromaticBondOrders(molecule);
@@ -96,12 +97,13 @@ public void testPyrrole_Silent() throws Exception {
         SmilesParser smilesParser = new SmilesParser(SilentChemObjectBuilder.getInstance());
         smilesParser.kekulise(false);
         IAtomContainer molecule = smilesParser.parseSmiles(smiles);
+        AtomContainerManipulator.setSingleOrDoubleFlags(molecule);
         AtomContainerManipulator.percieveAtomTypesAndConfigureAtoms(molecule);
-
         molecule = dbst.fixAromaticBondOrders(molecule);
         Assert.assertNotNull(molecule);
 
         molecule = (IAtomContainer) AtomContainerManipulator.removeHydrogens(molecule);
+
         int doubleBondCount = 0;
         for (int i = 0; i < molecule.getBondCount(); i++) {
             IBond bond = molecule.getBond(i);
@@ -164,6 +166,7 @@ public void testPyrrole_CustomRingFinder() throws Exception {
         SmilesParser smilesParser = new SmilesParser(DefaultChemObjectBuilder.getInstance());
         smilesParser.kekulise(false);
         IAtomContainer molecule = smilesParser.parseSmiles(smiles);
+        AtomContainerManipulator.setSingleOrDoubleFlags(molecule);
         AtomContainerManipulator.percieveAtomTypesAndConfigureAtoms(molecule);
 
         DeduceBondSystemTool dbst = new DeduceBondSystemTool(new AllRingsFinder());

diff --git a/storage/smiles/src/test/java/org/openscience/cdk/smiles/FixBondOrdersToolTest.java b/storage/smiles/src/test/java/org/openscience/cdk/smiles/FixBondOrdersToolTest.java
@@ -74,6 +74,7 @@ public void testPyrrole() throws Exception {
         SmilesParser smilesParser = new SmilesParser(DefaultChemObjectBuilder.getInstance());
         smilesParser.kekulise(false);
         IAtomContainer molecule = smilesParser.parseSmiles(smiles);
+        AtomContainerManipulator.setSingleOrDoubleFlags(molecule);
         AtomContainerManipulator.percieveAtomTypesAndConfigureAtoms(molecule);
 
         molecule = fbot.kekuliseAromaticRings(molecule);
@@ -95,11 +96,11 @@ public void testPyrrole_Silent() throws Exception {
         SmilesParser smilesParser = new SmilesParser(SilentChemObjectBuilder.getInstance());
         smilesParser.kekulise(false);
         IAtomContainer molecule = smilesParser.parseSmiles(smiles);
+        AtomContainerManipulator.setSingleOrDoubleFlags(molecule);
         AtomContainerManipulator.percieveAtomTypesAndConfigureAtoms(molecule);
 
         molecule = fbot.kekuliseAromaticRings(molecule);
         Assert.assertNotNull(molecule);
-
         molecule = (IAtomContainer) AtomContainerManipulator.removeHydrogens(molecule);
         int doubleBondCount = 0;
         for (int i = 0; i < molecule.getBondCount(); i++) {

diff --git a/storage/smiles/src/test/java/org/openscience/cdk/smiles/SmilesGeneratorTest.java b/storage/smiles/src/test/java/org/openscience/cdk/smiles/SmilesGeneratorTest.java
@@ -1199,6 +1199,13 @@ public void bug328() throws Exception {
                    is(canon("Clc1ccc(Cl)c2[nH]c([nH0]c21)C(F)(F)F")));
     }
 
+    @Test(expected = CDKException.class)
+    public void warnOnBadInput() throws Exception {
+        SmilesParser smipar = new SmilesParser(SilentChemObjectBuilder.getInstance());
+        smipar.kekulise(false);
+        IAtomContainer mol = smipar.parseSmiles("c1ccccc1");
+        System.err.println(SmilesGenerator.isomeric().create(mol));
+    }
 
     /**
      * @see https://tech.knime.org/forum/cdk/buggy-behavior-of-molecule-to-cdk-node

diff --git a/storage/smiles/src/test/java/org/openscience/cdk/smiles/SmilesParserTest.java b/storage/smiles/src/test/java/org/openscience/cdk/smiles/SmilesParserTest.java
@@ -151,7 +151,7 @@ public void testBug1363882() throws Exception {
     @Test(timeout = 1000)
     public void testBug1535587() throws Exception {
         String smiles = "COC(=O)c2ccc3n([H])c1ccccc1c3(c2)";
-        IAtomContainer mol = loadExact(smiles);
+        IAtomContainer mol = load(smiles);
         atomtype(mol);
         assertAtomTypesPerceived(mol);
         Assert.assertEquals(18, mol.getAtomCount());
@@ -164,7 +164,7 @@ public void testBug1535587() throws Exception {
     @Test(timeout = 1000)
     public void testBug1579235() throws Exception {
         String smiles = "c2cc1cccn1cc2";
-        IAtomContainer mol = loadExact(smiles);
+        IAtomContainer mol = load(smiles);
         atomtype(mol);
         assertAtomTypesPerceived(mol);
         Assert.assertEquals(9, mol.getAtomCount());
@@ -196,7 +196,7 @@ public void testBug1579229() throws Exception {
     @Test(timeout = 1000)
     public void testBug1579230() throws Exception {
         String smiles = "Cc1cccc2sc3nncn3c12";
-        IAtomContainer mol = loadExact(smiles);
+        IAtomContainer mol = load(smiles);
         atomtype(mol);
         assertAtomTypesPerceived(mol);
         Assert.assertEquals(13, mol.getAtomCount());
@@ -1554,13 +1554,13 @@ public void test3amino4methylpyridine() throws Exception {
     @org.junit.Test
     public void testPyrrole1() throws Exception {
         String smiles = "[nH]1cccc1";
-        IAtomContainer mol = loadExact(smiles);
+        IAtomContainer mol = load(smiles);
         atomtype(mol);
         assertAtomTypesPerceived(mol);
 
         Assert.assertEquals(5, mol.getAtomCount());
 
-        assertAllSingleAndAromatic(mol);
+        assertAllSingleOrAromatic(mol);
 
         assertAtomSymbols(new String[]{"N", "C", "C", "C", "C"}, mol);
 
@@ -1577,13 +1577,13 @@ public void testPyrrole1() throws Exception {
     @org.junit.Test
     public void testPyrrole2() throws Exception {
         String smiles = "n1([H])cccc1";
-        IAtomContainer mol = loadExact(smiles);
+        IAtomContainer mol = load(smiles);
         atomtype(mol);
         assertAtomTypesPerceived(mol);
 
         Assert.assertEquals(6, mol.getAtomCount());
 
-        assertAllSingleAndAromatic(mol);
+        assertAllSingleOrAromatic(mol);
 
         assertAtomSymbols(new String[]{"N", "H", "C", "C", "C", "C"}, mol);
 
@@ -1609,13 +1609,13 @@ public void testPyrrole3() throws Exception {
     @org.junit.Test
     public void testPyrroleAnion1() throws Exception {
         String smiles = "[n-]1cccc1";
-        IAtomContainer mol = loadExact(smiles);
+        IAtomContainer mol = load(smiles);
         atomtype(mol);
         assertAtomTypesPerceived(mol);
 
         Assert.assertEquals(5, mol.getAtomCount());
 
-        assertAllSingleAndAromatic(mol);
+        assertAllSingleOrAromatic(mol);
 
         assertAtomSymbols(new String[]{"N", "C", "C", "C", "C"}, mol);
 
@@ -1632,13 +1632,13 @@ public void testPyrroleAnion1() throws Exception {
     @org.junit.Test
     public void testImidazole1() throws Exception {
         String smiles = "[nH]1cncc1";
-        IAtomContainer mol = loadExact(smiles);
+        IAtomContainer mol = load(smiles);
         atomtype(mol);
         assertAtomTypesPerceived(mol);
 
         Assert.assertEquals(5, mol.getAtomCount());
 
-        assertAllSingleAndAromatic(mol);
+        assertAllSingleOrAromatic(mol);
 
         assertAtomSymbols(new String[]{"N", "C", "N", "C", "C"}, mol);
 
@@ -1655,13 +1655,13 @@ public void testImidazole1() throws Exception {
     @org.junit.Test
     public void testImidazole2() throws Exception {
         String smiles = "n1([H])cncc1";
-        IAtomContainer mol = loadExact(smiles);
+        IAtomContainer mol = load(smiles);
         atomtype(mol);
         assertAtomTypesPerceived(mol);
 
         Assert.assertEquals(6, mol.getAtomCount());
 
-        assertAllSingleAndAromatic(mol);
+        assertAllSingleOrAromatic(mol);
 
         assertAtomSymbols(new String[]{"N", "H", "C", "N", "C", "C"}, mol);
 
@@ -1687,13 +1687,13 @@ public void testImidazole3() throws Exception {
     @org.junit.Test
     public void testImidazole4() throws Exception {
         String smiles = "n1cc[nH]c1";
-        IAtomContainer mol = loadExact(smiles);
+        IAtomContainer mol = load(smiles);
         atomtype(mol);
         assertAtomTypesPerceived(mol);
 
         Assert.assertEquals(5, mol.getAtomCount());
 
-        assertAllSingleAndAromatic(mol);
+        assertAllSingleOrAromatic(mol);
 
         assertAtomSymbols(new String[]{"N", "C", "C", "N", "C"}, mol);
 
@@ -1730,13 +1730,13 @@ public void testPyridine1() throws Exception {
     @org.junit.Test
     public void testPyrimidine1() throws Exception {
         String smiles = "n1cnccc1";
-        IAtomContainer mol = loadExact(smiles);
+        IAtomContainer mol = load(smiles);
         atomtype(mol);
         assertAtomTypesPerceived(mol);
 
         Assert.assertEquals(6, mol.getAtomCount());
 
-        assertAllSingleAndAromatic(mol);
+        assertAllSingleOrAromatic(mol);
 
         assertAtomSymbols(new String[]{"N", "C", "N", "C", "C", "C"}, mol);
 
@@ -2431,7 +2431,7 @@ public void testPreserveAromaticityAndPerceiveAtomTypes() throws Exception {
     public void testAromaticBoron() throws Exception {
         IAtomContainer mol = loadExact("c1cc2c3cc1.c1cb23cc1");
         Assert.assertNotNull(mol);
-        assertAllSingleAndAromatic(mol);
+        assertAllSingleOrAromatic(mol);
     }
 
     /**

diff --git a/tool/sdg/src/main/java/org/openscience/cdk/layout/StructureDiagramGenerator.java b/tool/sdg/src/main/java/org/openscience/cdk/layout/StructureDiagramGenerator.java
@@ -2128,6 +2128,32 @@ private void placePositionalVariation(IAtomContainer mol) {
                     // get all atoms connected to the part we will move
                     Set<Integer> visited = new HashSet<>();
                     visit(visited, adjlist, atomIdx);
+
+                    // gather up other position group
+                    Set<Integer> newvisit = new HashSet<>();
+                    do {
+                        newvisit.clear();
+                        for (Integer idx : visited) {
+                            IAtom visitedAtom = mol.getAtom(idx);
+                            if (e.getKey().contains(visitedAtom) || e.getValue().contains(visitedAtom))
+                                continue;
+                            for (Map.Entry<Set<IAtom>, IAtom> e2 : mapping.entries()) {
+                                if (e2.getKey().contains(visitedAtom)) {
+                                    int other = idxs.get(e2.getValue());
+                                    if (!visited.contains(other) && newvisit.add(other)) {
+                                        visit(newvisit, adjlist, other);
+                                    }
+                                } else if (e2.getValue() == visitedAtom) {
+                                    int other = idxs.get(e2.getKey().iterator().next());
+                                    if (!visited.contains(other) && newvisit.add(other)) {
+                                        visit(newvisit, adjlist, other);
+                                    }
+                                }
+                            }
+                        }
+                        visited.addAll(newvisit);
+                    } while (!newvisit.isEmpty());
+
                     IAtomContainer frag = mol.getBuilder().newInstance(IAtomContainer.class);
                     for (Integer visit : visited)
                         frag.addAtom(mol.getAtom(visit));