Merge pull request #228 from cdk/patch/smarts-stereo-ringclosures

Patch/smarts stereo ringclosures
cdk · Aug 17, 2016 · cf60ea5 · cf60ea5
2 parents 6f9663f + d9a7de5
commit cf60ea5
Show file tree

Hide file tree

Showing 4 changed files with 72 additions and 48 deletions.
diff --git a/tool/smarts/src/main/java/org/openscience/cdk/smiles/smarts/SmartsPattern.java b/tool/smarts/src/main/java/org/openscience/cdk/smiles/smarts/SmartsPattern.java
@@ -189,6 +189,17 @@ public static SmartsPattern create(String smarts, IChemObjectBuilder builder) th
         return new SmartsPattern(smarts, builder);
     }
 
+    /**
+     * Default SMARTS pattern constructor, passes in a null chem object builder.
+     *
+     * @param smarts SMARTS pattern string
+     * @return a SMARTS pattern
+     * @throws IOException problem with SMARTS string syntax/semantics
+     */
+    public static SmartsPattern create(String smarts) throws IOException {
+        return new SmartsPattern(smarts, null);
+    }
+
     /**
      * Checks a smarts string for !R, R<num> or r<num>. If found then the more
      * expensive ring info needs to be initlised before querying.

diff --git a/tool/smarts/src/main/java/org/openscience/cdk/smiles/smarts/parser/SmartsQueryVisitor.java b/tool/smarts/src/main/java/org/openscience/cdk/smiles/smarts/parser/SmartsQueryVisitor.java
@@ -103,8 +103,6 @@ public class SmartsQueryVisitor implements SMARTSParserVisitor {
     // current atoms with a ring identifier
     private RingIdentifierAtom[]                ringAtoms;
 
-    private Multimap<IAtom, RingIdentifierAtom> ringAtomLookup = HashMultimap.create(10, 2);
-
     // query
     private IQueryAtomContainer                 query;
 
@@ -153,56 +151,63 @@ public Object visit(ASTRingIdentifier node, Object data) {
     public Object visit(ASTAtom node, Object data) {
         IQueryAtom atom = (IQueryAtom) node.jjtGetChild(0).jjtAccept(this, data);
         for (int i = 1; i < node.jjtGetNumChildren(); i++) { // if there are ring identifiers
-            ASTRingIdentifier ringIdentifier = (ASTRingIdentifier) node.jjtGetChild(i);
-            RingIdentifierAtom ringIdAtom = (RingIdentifierAtom) ringIdentifier.jjtAccept(this, atom);
+            throw new IllegalStateException();
+        }
+        return atom;
+    }
 
-            // if there is already a RingIdentifierAtom, create a bond between
-            // them and add the bond to the query
-            int ringId = ringIdentifier.getRingId();
+    private void handleRingClosure(IQueryAtom atom, ASTRingIdentifier ringIdentifier) {
+        RingIdentifierAtom ringIdAtom = (RingIdentifierAtom) ringIdentifier.jjtAccept(this, atom);
 
-            // ring digit > 9 - expand capacity
-            if (ringId >= ringAtoms.length) ringAtoms = Arrays.copyOf(ringAtoms, 100);
+        // if there is already a RingIdentifierAtom, create a bond between
+        // them and add the bond to the query
+        int ringId = ringIdentifier.getRingId();
 
-            // Ring Open
-            if (ringAtoms[ringId] == null) {
-                ringAtoms[ringId] = ringIdAtom;
-                ringAtomLookup.put(atom, ringIdAtom);
+        // ring digit > 9 - expand capacity
+        if (ringId >= ringAtoms.length) ringAtoms = Arrays.copyOf(ringAtoms, 100);
+
+        // Ring Open
+        if (ringAtoms[ringId] == null) {
+            ringAtoms[ringId] = ringIdAtom;
+            if (neighbors.containsKey(atom)) {
+                neighbors.get(atom).add(ringIdAtom);
             }
+        }
 
-            // Ring Close
-            else {
-                IQueryBond ringBond;
-                // first check if the two bonds ma
-                if (ringAtoms[ringId].getRingBond() == null) {
-                    if (ringIdAtom.getRingBond() == null) {
-                        if (atom instanceof AromaticSymbolAtom
-                                && ringAtoms[ringId].getAtom() instanceof AromaticSymbolAtom) {
-                            ringBond = new AromaticQueryBond(builder);
-                        } else {
-                            ringBond = new RingBond(builder);
-                        }
+        // Ring Close
+        else {
+            IQueryBond ringBond;
+            // first check if the two bonds ma
+            if (ringAtoms[ringId].getRingBond() == null) {
+                if (ringIdAtom.getRingBond() == null) {
+                    if (atom instanceof AromaticSymbolAtom
+                            && ringAtoms[ringId].getAtom() instanceof AromaticSymbolAtom) {
+                        ringBond = new AromaticQueryBond(builder);
                     } else {
-                        ringBond = ringIdAtom.getRingBond();
+                        ringBond = new RingBond(builder);
                     }
                 } else {
-                    // Here I assume the bond are always same. This should be checked by the parser already
-                    ringBond = ringAtoms[ringId].getRingBond();
+                    ringBond = ringIdAtom.getRingBond();
                 }
-                ((IBond) ringBond).setAtoms(new IAtom[]{ringAtoms[ringId].getAtom(), atom});
-                query.addBond((IBond) ringBond);
-
-                // if the connected atoms was tracking neighbors, replace the
-                // placeholder reference
-                if (neighbors.containsKey(ringAtoms[ringId].getAtom())) {
-                    List<IAtom> localNeighbors = neighbors.get(ringAtoms[ringId].getAtom());
-                    localNeighbors.set(localNeighbors.indexOf(ringAtoms[ringId]), atom);
-                }
-
-                ringAtomLookup.remove(ringAtoms[ringId].getAtom(), ringIdAtom);
-                ringAtoms[ringId] = null;
+            } else {
+                // Here I assume the bond are always same. This should be checked by the parser already
+                ringBond = ringAtoms[ringId].getRingBond();
+            }
+            ((IBond) ringBond).setAtoms(new IAtom[]{ringAtoms[ringId].getAtom(), atom});
+            query.addBond((IBond) ringBond);
+
+            // if the connected atoms was tracking neighbors, replace the
+            // placeholder reference
+            if (neighbors.containsKey(ringAtoms[ringId].getAtom())) {
+                List<IAtom> localNeighbors = neighbors.get(ringAtoms[ringId].getAtom());
+                localNeighbors.set(localNeighbors.indexOf(ringAtoms[ringId]), atom);
             }
+            if (neighbors.containsKey(atom)) {
+                neighbors.get(atom).add(ringAtoms[ringId].getAtom());
+            }
+
+            ringAtoms[ringId] = null;
         }
-        return atom;
     }
 
     private final static ILoggingTool logger = LoggingToolFactory.createLoggingTool(SmartsQueryVisitor.class);
@@ -360,17 +365,17 @@ public Object visit(ASTSmarts node, Object data) {
             query.addBond(bond);
             bond = null;
         }
+
+        // first ATOM in expresion
         query.addAtom(atom);
 
         if (tetrahedral.get(query.getAtomCount() - 1)) {
             List<IAtom> localNeighbors = new ArrayList<IAtom>(query.getConnectedAtomsList(atom));
             localNeighbors.add(atom);
-            // placeholders for ring closure
-            for (RingIdentifierAtom ringIdAtom : ringAtomLookup.get(atom))
-                localNeighbors.add(ringIdAtom);
             neighbors.put(atom, localNeighbors);
         }
 
+        // now process the rest of the bonds/atoms
         for (int i = 1; i < node.jjtGetNumChildren(); i++) {
             Node child = node.jjtGetChild(i);
             if (child instanceof ASTLowAndBond) {
@@ -390,9 +395,6 @@ public Object visit(ASTSmarts node, Object data) {
                 if (tetrahedral.get(query.getAtomCount() - 1)) {
                     List<IAtom> localNeighbors = new ArrayList<IAtom>(query.getConnectedAtomsList(newAtom));
                     localNeighbors.add(newAtom);
-                    // placeholders for ring closure
-                    for (RingIdentifierAtom ringIdAtom : ringAtomLookup.get(newAtom))
-                        localNeighbors.add(ringIdAtom);
                     neighbors.put(newAtom, localNeighbors);
                 }
 
@@ -401,6 +403,10 @@ public Object visit(ASTSmarts node, Object data) {
             } else if (child instanceof ASTSmarts) { // another smarts
                 child.jjtAccept(this, new Object[]{atom, bond});
                 bond = null;
+            } else if (child instanceof ASTRingIdentifier) {
+                handleRingClosure(atom, (ASTRingIdentifier) child);
+            } else {
+                throw new IllegalStateException("Unhandled node type: " + child.getClass());
             }
         }
 

diff --git a/tool/smarts/src/main/jjtree/org/openscience/cdk/smiles/smarts/parser/SMARTSParser.jjt b/tool/smarts/src/main/jjtree/org/openscience/cdk/smiles/smarts/parser/SMARTSParser.jjt
@@ -425,7 +425,7 @@ void SmartsExpression() #Smarts :
 					ringId.jjtAddChild(bond, 0);
 				}
 				ringId.setRingId(ringIdToken);
-				atom.jjtAddChild(ringId, atom.jjtGetNumChildren());
+				jjtree.pushNode(ringId);
 	  		}	
 	  		|
 	  		atom = AtomExpression()

diff --git a/tool/smarts/src/test/java/org/openscience/cdk/smiles/smarts/SmartsPatternTest.java b/tool/smarts/src/test/java/org/openscience/cdk/smiles/smarts/SmartsPatternTest.java
@@ -29,6 +29,7 @@
 import org.openscience.cdk.interfaces.IAtomContainer;
 import org.openscience.cdk.interfaces.IChemObjectBuilder;
 import org.openscience.cdk.interfaces.IReaction;
+import org.openscience.cdk.isomorphism.Pattern;
 import org.openscience.cdk.silent.SilentChemObjectBuilder;
 import org.openscience.cdk.smiles.SmilesParser;
 
@@ -182,6 +183,12 @@ public void mismatchedQueryMapsIgnored() throws Exception {
                    is(2));
     }
 
+    @Test
+    public void stereo_ring_closures() throws Exception {
+        Pattern ptrn = SmartsPattern.create("[C@@]1(O[C@@]([C@@]([C@]([C@]1(C)O)(C)O)(O)C)(O)C)(O)C");
+        assertTrue(ptrn.matches(smi("[C@@]1(O[C@@]([C@@]([C@]([C@]1(C)O)(C)O)(O)C)(O)C)(O)C")));
+    }
+
     IAtomContainer smi(String smi) throws Exception {
         return new SmilesParser(bldr).parseSmiles(smi);
     }