A SMILES should match it's self as a SMARTS, this example did not. Th…

…e reason was the stereochemistry ordering was read incorrectly due to how rings were handled in the SMARTS parser, to fix the nesting of the syntax tree needed to be changed.
cdk · Aug 16, 2016 · d9a7de5 · d9a7de5
1 parent 67ffd0a
commit d9a7de5
Show file tree

Hide file tree

Showing 3 changed files with 61 additions and 48 deletions.
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);
     }