Merge pull request #191 from cdk/01feb16

Fix bug where by stereochemistry was ignored when replacing and atom using the manipulator.
cdk · Feb 12, 2016 · a838988 · a838988
2 parents 84ebe07 + 38abffc
commit a838988
Show file tree

Hide file tree

Showing 3 changed files with 62 additions and 30 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
@@ -60,6 +60,7 @@
 import org.openscience.cdk.interfaces.ITetrahedralChirality;
 import org.openscience.cdk.ringsearch.RingSearch;
 import org.openscience.cdk.stereo.DoubleBondStereochemistry;
+import org.openscience.cdk.stereo.ExtendedTetrahedral;
 import org.openscience.cdk.stereo.TetrahedralChirality;
 
 /**
@@ -126,38 +127,52 @@ public static IAtom getAtomById(IAtomContainer ac, String id) throws CDKExceptio
         throw new CDKException("no suc atom");
     }
 
-    public static boolean replaceAtomByAtom(IAtomContainer container, IAtom atom, IAtom newAtom) {
-        if (!container.contains(atom)) {
-            // it should complain
-            return false;
-        } else {
-            container.setAtom(container.getAtomNumber(atom), newAtom);
-            for (IElectronContainer eContainer : container.electronContainers()) {
-                if (eContainer instanceof IBond) {
-                    IBond bond = (IBond) eContainer;
-                    if (bond.contains(atom)) {
-                        for (int j = 0; j < bond.getAtomCount(); j++) {
-                            if (atom.equals(bond.getAtom(j))) {
-                                bond.setAtom(newAtom, j);
-                                break;
-                            }
-                        }
-                    }
-                } else if (eContainer instanceof ILonePair) {
-                    ILonePair lonePair = (ILonePair) eContainer;
-                    if (atom.equals(lonePair.getAtom())) {
-                        lonePair.setAtom(newAtom);
-                    }
-                } else if (eContainer instanceof ISingleElectron) {
-                    ISingleElectron singleElectron = (ISingleElectron) eContainer;
-                    if (atom.equals(singleElectron.getAtom())) {
-                        singleElectron.setAtom(newAtom);
-                    }
-                }
+    /**
+     * Substitute one atom in a container for another adjusting bonds, single electrons, lone pairs, and stereochemistry
+     * as required.
+     *
+     * @param container the container to replace the atom of
+     * @param oldAtom the atom to replace
+     * @param newAtom the atom to insert
+     * @return whether replacement was made
+     */
+    public static boolean replaceAtomByAtom(final IAtomContainer container, final IAtom oldAtom, final IAtom newAtom) {
+
+        Map<IAtom,IAtom> atomremap = new HashMap<>();
+
+        for (int i = 0; i < container.getAtomCount(); i++) {
+            IAtom atom = container.getAtom(i);
+            if (atom == oldAtom) {
+                container.setAtom(i, newAtom);
+                atomremap.put(oldAtom, newAtom);
+            } else {
+                atomremap.put(atom, atom);
             }
-            return true;
         }
-        // FIXME - also need to handle stereochemistry
+
+        if (!atomremap.containsKey(oldAtom))
+            return false;
+
+        Map<IBond,IBond> bondremap = new HashMap<>();
+        for (IBond bond : container.bonds()) {
+            bondremap.put(bond, bond);
+            for (int i = 0; i < bond.getAtomCount(); i++)
+                if (bond.getAtom(i) == oldAtom)
+                    bond.setAtom(newAtom, i);
+        }
+        for (ISingleElectron ec : container.singleElectrons())
+            if (ec.getAtom() == oldAtom)
+                ec.setAtom(newAtom);
+        for (ILonePair lp : container.lonePairs())
+            if (lp.getAtom() == oldAtom)
+                lp.setAtom(newAtom);
+
+        List<IStereoElement> stereoremapped = new ArrayList<>();
+        for (IStereoElement se : container.stereoElements())
+            stereoremapped.add(se.map(atomremap, bondremap));
+        container.setStereoElements(stereoremapped);
+
+        return true;
     }
 
     /**

diff --git a/...ard/src/test/java/org/openscience/cdk/tools/manipulator/AtomContainerManipulatorTest.java b/...ard/src/test/java/org/openscience/cdk/tools/manipulator/AtomContainerManipulatorTest.java
@@ -912,6 +912,17 @@ public void testReplaceAtom_singleElectron() {
         Assert.assertEquals(atom3, container.getSingleElectron(0).getAtom());
     }
 
+    @Test
+    public void testReplaceAtom_stereochemistry() throws Exception {
+        IChemObjectBuilder bldr = SilentChemObjectBuilder.getInstance();
+        SmilesParser smipar = new SmilesParser(bldr);
+        IAtomContainer mol = smipar.parseSmiles("N[C@H](CC)O");
+        IAtom newAtom = bldr.newInstance(IAtom.class, "Cl");
+        newAtom.setImplicitHydrogenCount(0);
+        AtomContainerManipulator.replaceAtomByAtom(mol, mol.getAtom(0), newAtom);
+        assertThat(SmilesGenerator.isomeric().create(mol), is("Cl[C@H](CC)O"));
+    }
+
     @Test
     public void testGetHeavyAtoms_IAtomContainer() {
         IChemObjectBuilder builder = DefaultChemObjectBuilder.getInstance();

diff --git a/tool/sdg/src/main/java/org/openscience/cdk/layout/NonplanarBonds.java b/tool/sdg/src/main/java/org/openscience/cdk/layout/NonplanarBonds.java
@@ -437,6 +437,12 @@ boolean hasPriority(int focus, int i, int j) {
         if (!iCyclic && jCyclic) return true;
         if (iCyclic && !jCyclic) return false;
 
+        // avoid placing on pseudo atoms
+        if (container.getAtom(i).getAtomicNumber() > 0 && container.getAtom(j).getAtomicNumber() == 0)
+            return true;
+        if (container.getAtom(i).getAtomicNumber() == 0 && container.getAtom(j).getAtomicNumber() > 0)
+            return false;
+
         // prioritise atoms with fewer neighbors
         if (graph[i].length < graph[j].length) return true;
         if (graph[i].length > graph[j].length) return false;