Add a boolean which returns whether or not something was changed by a SsurgeonEdit evaluation. Add a bunch of notes to the operations, including some notes on whether or not they are used in RTE

Author: AngledLuffa

src/edu/stanford/nlp/semgraph/semgrex/ssurgeon/AddDep.java

@@ -80,9 +80,11 @@ public String toEditString() {
    * TODO: figure out how to specify where in the sentence this node goes.
    * TODO: determine if we should be copying an IndexedWord, or working just with a FeatureLabel.
    * TODO: bombproof if this gov, dep, and reln already exist.
+   * TODO: This is not used anywhere, even in the old RTE code, so we can redo it however we want.
+   *       Perhaps it could reorder the indices of the new nodes, for example
    */
   @Override
-  public void evaluate(SemanticGraph sg, SemgrexMatcher sm) {
+  public boolean evaluate(SemanticGraph sg, SemgrexMatcher sm) {
     IndexedWord govNode = sm.getNode(govNodeName);
     IndexedWord newNode = new IndexedWord(newNodePrototype);
     int newIndex = SemanticGraphUtils.leftMostChildVertice(govNode, sg).index(); // cheap En-specific hack for placing copula (beginning of governing phrase)
@@ -91,6 +93,7 @@ public void evaluate(SemanticGraph sg, SemgrexMatcher sm) {
     newNode.setSentIndex(govNode.sentIndex());
     sg.addVertex(newNode);
     sg.addEdge(govNode, newNode, relation, weight,false);
+    return true;
   }
 
   public static final String WORD_KEY = "word";

src/edu/stanford/nlp/semgraph/semgrex/ssurgeon/AddEdge.java

@@ -65,21 +65,26 @@ public static AddEdge createEngAddEdge(String govName, String depName, String en
     return new AddEdge(govName, depName, reln, weight);
   }
 
-  
+
+  /**
+   * If the edge already exists in the graph,
+   * a new edge is not added.
+   */
   @Override
-  public void evaluate(SemanticGraph sg, SemgrexMatcher sm) {
+  public boolean evaluate(SemanticGraph sg, SemgrexMatcher sm) {
     IndexedWord govNode = getNamedNode(govName, sm);
     IndexedWord depNode =  getNamedNode(depName, sm);
     SemanticGraphEdge existingEdge = sg.getEdge(govNode, depNode, relation);
     if (existingEdge == null) {
       // When adding the edge, check to see if the gov/dep nodes are presently in the graph.
-      // 
       if (!sg.containsVertex(govNode)) 
         sg.addVertex(govNode);
       if (!sg.containsVertex(depNode)) 
         sg.addVertex(depNode);
       sg.addEdge(govNode, depNode, relation, weight,false );
+      return true;
     }
+    return false;
   }
 
 }

src/edu/stanford/nlp/semgraph/semgrex/ssurgeon/AddNode.java

@@ -20,16 +20,23 @@ public static AddNode createAddNode(String nodeString, String nodeName) {
     return new AddNode(nodeString, nodeName);
   }
 
-public static AddNode createAddNode(IndexedWord node, String nodeName) {
-  String nodeString = AddDep.cheapWordToString(node);
-  return new AddNode(nodeString, nodeName);
-}
+  public static AddNode createAddNode(IndexedWord node, String nodeName) {
+    String nodeString = AddDep.cheapWordToString(node);
+    return new AddNode(nodeString, nodeName);
+  }
 
+  // TODO: can this be bombproofed if the node is already added?
+  // otherwise, we can insist the user make sure the
+  // node doesn't already exist, similar to Tsurgeon
+  // Alternatively we could just not export this one and
+  // make AddDep a bit more configurable.
+  // This one is actually used in its current form in RTE
   @Override
-  public void evaluate(SemanticGraph sg, SemgrexMatcher sm) {
+  public boolean evaluate(SemanticGraph sg, SemgrexMatcher sm) {
     IndexedWord newNode = AddDep.fromCheapString(nodeString);
     sg.addVertex(newNode);
     addNamedNode(newNode, nodeName);
+    return true;
   }
 
 

src/edu/stanford/nlp/semgraph/semgrex/ssurgeon/CollapseSubtree.java

@@ -38,12 +38,18 @@ public CollapseSubtree(String rootNodeName) {
 
   @SuppressWarnings({ "unchecked", "rawtypes" })
   @Override
-  public void evaluate(SemanticGraph sg, SemgrexMatcher sm) {
-
+  public boolean evaluate(SemanticGraph sg, SemgrexMatcher sm) {
     IndexedWord rootNode = this.getNamedNode(rootName, sm);
     Set<IndexedWord> subgraphNodeSet = sg.getSubgraphVertices(rootNode);
+    if (subgraphNodeSet.size() == 1) {
+      // our work here is done
+      return false;
+    }
 
-
+    // TODO: this doesn't do a full search for cycles.  Is that relevant?
+    // Why does this even matter?  Perhaps the only thing we care about
+    // is that the root of the whole graph isn't collapsed
+    // unless it stays the root
     if ( ! sg.isDag(rootNode)) {
       /* Check if there is a cycle going back to the root. */
       for (IndexedWord child : sg.getChildren(rootNode)) {
@@ -80,6 +86,7 @@ public void evaluate(SemanticGraph sg, SemgrexMatcher sm) {
       sg.removeVertex(node);
     }
 
+    return true;
   }
 
   @Override

src/edu/stanford/nlp/semgraph/semgrex/ssurgeon/DeleteGraphFromNode.java

@@ -64,18 +64,21 @@ protected static Set<IndexedWord> crawl(IndexedWord vertex, SemanticGraph sg) {
 
 
   @Override
-  public void evaluate(SemanticGraph sg, SemgrexMatcher sm) {
+  public boolean evaluate(SemanticGraph sg, SemgrexMatcher sm) {
     IndexedWord seedNode = getNamedNode(destroyNodeName, sm);
-    // TODO: do not execute if seedNode if not in graph (or just error?)
-    if (sg.containsVertex(seedNode)) {
-      Set<IndexedWord> nodesToDestroy = crawl(seedNode, sg);
-      for (IndexedWord node : nodesToDestroy) {
-        sg.removeVertex(node);
-      }
-      // After destroy nodes, need to reset the roots, since it's possible a root node
-      // was destroyed.
-      sg.resetRoots();
+    if (seedNode == null || !sg.containsVertex(seedNode)) {
+      return false;
+    }
+
+    Set<IndexedWord> nodesToDestroy = crawl(seedNode, sg);
+    for (IndexedWord node : nodesToDestroy) {
+      sg.removeVertex(node);
     }
+    // After destroy nodes, need to reset the roots, since it's possible a root node
+    // was destroyed.
+    // TODO: better is to check first
+    sg.resetRoots();
+    return true;
   }
 
 }

src/edu/stanford/nlp/semgraph/semgrex/ssurgeon/KillAllIncomingEdges.java

@@ -20,12 +20,21 @@ public KillAllIncomingEdges(String nodeName) {
     this.nodeName = nodeName;
   }
 
+  /**
+   * If executed twice on the same node, the second time there
+   * will be no further updates
+   */
   @Override
-  public void evaluate(SemanticGraph sg, SemgrexMatcher sm) {
-   IndexedWord tgtNode = getNamedNode(nodeName, sm);
-   for (SemanticGraphEdge edge : sg.incomingEdgeIterable(tgtNode)) {
-     sg.removeEdge(edge);
-   }
+  public boolean evaluate(SemanticGraph sg, SemgrexMatcher sm) {
+    IndexedWord tgtNode = getNamedNode(nodeName, sm);
+    if (tgtNode == null) {
+      return false;
+    }
+    boolean success = false;
+    for (SemanticGraphEdge edge : sg.incomingEdgeIterable(tgtNode)) {
+      success = success || sg.removeEdge(edge);
+    }
+    return success;
   }
 
   @Override

src/edu/stanford/nlp/semgraph/semgrex/ssurgeon/KillNonRootedNodes.java

@@ -16,16 +16,22 @@
  */
 public class KillNonRootedNodes extends SsurgeonEdit {
   public static final String LABEL = "killNonRooted"; 
-  
+
+  /**
+   * If executed twice on the same graph, the second time there
+   * will be no further updates
+   */
   @Override
-  public void evaluate(SemanticGraph sg, SemgrexMatcher sm) {
+  public boolean evaluate(SemanticGraph sg, SemgrexMatcher sm) {
+    boolean changed = false;
     List<IndexedWord> nodes = new ArrayList<>(sg.vertexSet());
     for (IndexedWord node : nodes) {
       List<IndexedWord> rootPath = sg.getPathToRoot(node);
       if (rootPath == null) {
-        sg.removeVertex(node);
+        changed = changed || sg.removeVertex(node);
       }
     }
+    return changed;
   }
 
   @Override

src/edu/stanford/nlp/semgraph/semgrex/ssurgeon/RemoveEdge.java

@@ -42,34 +42,51 @@ public String toEditString() {
 
   public static final String WILDCARD_NODE = "**WILDNODE**";
 
+  /**
+   * Remove all edges from gov to dep that match the relation name.
+   *<br>
+   * Either gov or dep can be **WILDNODE**, representing that all
+   * edges to or from the other node of that type should be removed.
+   *<br>
+   * You cannot set both gov and dep to **WILDNODE**, though.
+   *<br>
+   * This will not update anything the second time executed on the
+   * same graph.
+   */
   @Override
-  public void evaluate(SemanticGraph sg, SemgrexMatcher sm) {
+  public boolean evaluate(SemanticGraph sg, SemgrexMatcher sm) {
     boolean govWild = govName.equals(WILDCARD_NODE);
     boolean depWild = depName.equals(WILDCARD_NODE);
     IndexedWord govNode = getNamedNode(govName, sm);
-    IndexedWord depNode =getNamedNode(depName, sm);
+    IndexedWord depNode = getNamedNode(depName, sm);
+    boolean success = false;
 
     if (govNode != null && depNode != null) {
       SemanticGraphEdge edge = sg.getEdge(govNode, depNode, relation);
-      if (edge != null) {
-        @SuppressWarnings("unused")
-        boolean successFlag = sg.removeEdge(edge);
+      while (edge != null) {
+        if (!sg.removeEdge(edge)) {
+          throw new IllegalStateException("Found an edge and tried to delete it, but somehow this didn't work!  " + edge);
+        }
+        edge = sg.getEdge(govNode, depNode, relation);
+        success = true;
       }
     } else if (depNode != null && govWild) {
       // dep known, wildcard gov
       for (SemanticGraphEdge edge : sg.incomingEdgeIterable(depNode)) {
         if (edge.getRelation().equals(relation) && sg.containsEdge(edge) ) {
-          sg.removeEdge(edge);
+          success = success || sg.removeEdge(edge);
         }
       }
     }  else if (govNode != null && depWild) {
       // gov known, wildcard dep
       for (SemanticGraphEdge edge : sg.outgoingEdgeIterable(govNode)) {
         if (edge.getRelation().equals(relation) && sg.containsEdge(edge) ) {
-          sg.removeEdge(edge);
+          success = success || sg.removeEdge(edge);
         }
       }
     }
+    // will be true if at least one edge was removed
+    return success;
   }
 
 

src/edu/stanford/nlp/semgraph/semgrex/ssurgeon/RemoveNamedEdge.java

@@ -45,17 +45,27 @@ public String toEditString() {
     buf.write(depName);
     return buf.toString();
   }
-  
+
+  /**
+   * Removes the named edge from the graph, if it exists.
+   *<br>
+   * TODO: it should not be necessary to have node names for this to work.
+   * Any edge that gets matched by the edge matcher should just work
+   * This operation is not used anywhere, even in RTE, so we should be
+   * able to change its semantics to not include the node names
+   */
   @Override
-  public void evaluate(SemanticGraph sg, SemgrexMatcher sm) {    
+  public boolean evaluate(SemanticGraph sg, SemgrexMatcher sm) {
     String relation = sm.getRelnString(edgeName);
     IndexedWord govNode = getNamedNode(govName, sm);
     IndexedWord depNode = getNamedNode(depName, sm);
     SemanticGraphEdge edge = sg.getEdge(govNode, depNode, GrammaticalRelation.valueOf(relation));
-    
+
     if (edge != null) {
       sg.removeEdge(edge);
+      return true;
     }
+    return false;
   }
 
   public String getDepName() {

src/edu/stanford/nlp/semgraph/semgrex/ssurgeon/SetRoots.java

@@ -18,13 +18,20 @@ public class SetRoots extends SsurgeonEdit {
   public SetRoots(List<String> newRootNames) {
     this.newRootNames = newRootNames;
   }
-  
+
+  /**
+   * If executed twice on the same graph, the second time there
+   * will be no further updates
+   */
   @Override
-  public void evaluate(SemanticGraph sg, SemgrexMatcher sm) {
-    List<IndexedWord> newRoots = new ArrayList<>();
+  public boolean evaluate(SemanticGraph sg, SemgrexMatcher sm) {
+    Set<IndexedWord> newRoots = new LinkedHashSet<>();
     for (String name : newRootNames)
       newRoots.add(getNamedNode(name, sm));
-    sg.setRoots(newRoots);    
+    if (newRoots.equals(sg.getRoots()))
+      return false;
+    sg.setRoots(newRoots);
+    return true;
   }
 
   @Override