Add the capacity for MergeNodes to merge nodes with the same head. Useful for a case we found with two parts of a time that was too tokenized were pointing to the same head, rather than the time being one self-contained phrase

AngledLuffa · AngledLuffa · commit 4f915f1b70ed · 2025-06-16T14:23:09.000-07:00
diff --git a/src/edu/stanford/nlp/semgraph/semgrex/ssurgeon/MergeNodes.java b/src/edu/stanford/nlp/semgraph/semgrex/ssurgeon/MergeNodes.java
@@ -15,6 +15,8 @@
 import edu.stanford.nlp.semgraph.SemanticGraph;
 import edu.stanford.nlp.semgraph.SemanticGraphEdge;
 import edu.stanford.nlp.semgraph.semgrex.SemgrexMatcher;
+import edu.stanford.nlp.trees.GrammaticalRelation;
+import edu.stanford.nlp.util.Pair;
 
 /**
  * Combines two words into one word
@@ -60,6 +62,31 @@ public String toEditString() {
     return buf.toString();
   }
 
+  /**
+   * Test if two nodes have the same parents with the same relations.
+   * If so, then the two nodes can be treated as equivalent when merging nodes.
+   * Otherwise, since there are two different heads, we can't pick a node
+   * to treat as the head of the phrase, and we will have to abort
+   */
+  public static boolean hasSameParents(SemanticGraph sg, IndexedWord head, IndexedWord candidate, Set<IndexedWord> ignoreNodes) {
+    Set<Pair<IndexedWord, GrammaticalRelation>> headParents = new HashSet<>();
+    Set<Pair<IndexedWord, GrammaticalRelation>> candidateParents = new HashSet<>();
+
+    for (SemanticGraphEdge edge : sg.incomingEdgeIterable(head)) {
+      // iterating all parents is relevant for enhanced graphs, for example
+      if (ignoreNodes.contains(edge.getGovernor()))
+        continue;
+      headParents.add(new Pair<>(edge.getGovernor(), edge.getRelation()));
+    }
+    for (SemanticGraphEdge edge : sg.incomingEdgeIterable(candidate)) {
+      // iterating all parents is relevant for enhanced graphs, for example
+      if (ignoreNodes.contains(edge.getGovernor()))
+        continue;
+      candidateParents.add(new Pair<>(edge.getGovernor(), edge.getRelation()));
+    }
+    return headParents.equals(candidateParents);
+  }
+
   /**
    * If the named nodes are next to each other, and the edges of
    * the graph allow for it, squish those words into one word
@@ -76,6 +103,12 @@ public boolean evaluate(SemanticGraph sg, SemgrexMatcher sm) {
     }
 
     IndexedWord head = null;
+    // Words who share the same parents will go in this set
+    // Therefore, we can later remap any edges going to that word
+    // to point to the chosen head instead
+    // This will let us process phrases where two words could have
+    // been the head and both have edges coming in to them
+    Set<IndexedWord> equivalentHeads = new HashSet<>();
     for (IndexedWord candidate : nodeSet) {
       Set<IndexedWord> parents = sg.getParents(candidate);
       if (parents.size() == 0) {
@@ -96,9 +129,10 @@ public boolean evaluate(SemanticGraph sg, SemgrexMatcher sm) {
         // parent is outside this subtree
         // therefore, we can use this word as the head of the subtree
         if (head != null) {
-          if (parents.equals(sg.getParents(head))) {
-            // if the parents of the other node are the same, we can keep going
+          if (hasSameParents(sg, head, candidate, nodeSet)) {
+            // if the parents *and relations* of the other node are the same, we can keep going
             // since the nodes are about to merge anyway
+            equivalentHeads.add(candidate);
             break;
           } else {
             // if we already have a head with different parents, give up instead
@@ -114,18 +148,36 @@ public boolean evaluate(SemanticGraph sg, SemgrexMatcher sm) {
     }
 
     // for now, only allow the head to have edges to children outside the subtree
-    // TODO: instead, could make them all point to the new merged word...
-    // but it's not clear that's a structure we want to allow merged
+    // also, words with the same parents as the new head can have outgoing edges
+    // TODO: not clear we want to allow other words with different
+    // heads to be merged in this manner
+    List<SemanticGraphEdge> reattachEdges = new ArrayList<>();
     for (IndexedWord candidate : nodeSet) {
       if (candidate == head) {
         continue;
       }
-      for (IndexedWord child : sg.getChildren(candidate)) {
-        if (!nodeSet.contains(child)) {
-          return false;
+      for (SemanticGraphEdge edge : sg.outgoingEdgeIterable(candidate)) {
+        IndexedWord gov = edge.getGovernor();
+        if (gov != candidate) {
+          throw new AssertionError();
+        }
+        IndexedWord dep = edge.getDependent();
+        if (!nodeSet.contains(dep)) {
+          if (equivalentHeads.contains(candidate)) {
+            reattachEdges.add(edge);
+          } else {
+            return false;
+          }
         }
       }
     }
+
+    // at this point, everything checks out and we can start manipulating the graph
+    // we will start by reattaching incoming edges to the chosen head
+    for (SemanticGraphEdge edge : reattachEdges) {
+      ReattachNamedEdge.reattachEdge(sg, sm, edge, null, head, edge.getDependent());
+    }
+
     ArrayList<IndexedWord> nodes = new ArrayList<>(nodeSet);
     Collections.sort(nodes);
 
diff --git a/src/edu/stanford/nlp/semgraph/semgrex/ssurgeon/ReattachNamedEdge.java b/src/edu/stanford/nlp/semgraph/semgrex/ssurgeon/ReattachNamedEdge.java
@@ -59,6 +59,45 @@ public String toEditString() {
     return buf.toString();
   }
 
+  public static boolean reattachEdge(SemanticGraph sg, SemgrexMatcher sm,
+                                     SemanticGraphEdge edge, String edgeName, IndexedWord gov, IndexedWord dep) {
+    if (gov == edge.getSource() && dep == edge.getTarget()) {
+      // we were asked to point the edge to the same nodes it already pointed to
+      // nothing to do
+      return false;
+    }
+    boolean success = sg.removeEdge(edge);
+    if (!success) {
+      // maybe it was already removed somehow by a previous operation
+      return false;
+    }
+    final SemanticGraphEdge newEdge;
+    found: {
+      for (SemanticGraphEdge existingEdge : sg.getAllEdges(edge.getSource(), edge.getTarget())) {
+        if (existingEdge.getRelation().equals(edge.getRelation())) {
+          newEdge = existingEdge;
+          break found;
+        }
+      }
+      newEdge = new SemanticGraphEdge(gov,
+                                      dep,
+                                      edge.getRelation(),
+                                      edge.getWeight(),
+                                      edge.isExtra());
+      sg.addEdge(newEdge);
+    }
+    // whether we recreated a new edge with the new relation,
+    // or found an existing edge with the relation we wanted,
+    // update the named edge in the SemgrexMatcher so future
+    // iterations have the name connected to the edge
+    // TODO: if an existing edge was clobbered, perhaps we need to
+    // update anything that named it
+    if (edgeName != null) {
+      sm.putNamedEdge(edgeName, newEdge);
+    }
+    return true;
+  }
+
   /**
    * "Reattach" the named edge by removing it and then recreating it with the new gov and/or dep
    */
@@ -69,39 +108,7 @@ public boolean evaluate(SemanticGraph sg, SemgrexMatcher sm) {
     if (edge != null) {
       final IndexedWord gov = (govNodeName != null) ? sm.getNode(govNodeName) : edge.getSource();
       final IndexedWord dep = (depNodeName != null) ? sm.getNode(depNodeName) : edge.getTarget();
-      if (gov == edge.getSource() && dep == edge.getTarget()) {
-        // we were asked to point the edge to the same nodes it already pointed to
-        // nothing to do
-        return false;
-      }
-      boolean success = sg.removeEdge(edge);
-      if (!success) {
-        // maybe it was already removed somehow by a previous operation
-        return false;
-      }
-      final SemanticGraphEdge newEdge;
-      found: {
-        for (SemanticGraphEdge existingEdge : sg.getAllEdges(edge.getSource(), edge.getTarget())) {
-          if (existingEdge.getRelation().equals(edge.getRelation())) {
-            newEdge = existingEdge;
-            break found;
-          }
-        }
-        newEdge = new SemanticGraphEdge(gov,
-                                        dep,
-                                        edge.getRelation(),
-                                        edge.getWeight(),
-                                        edge.isExtra());
-        sg.addEdge(newEdge);
-      }
-      // whether we recreated a new edge with the new relation,
-      // or found an existing edge with the relation we wanted,
-      // update the named edge in the SemgrexMatcher so future
-      // iterations have the name connected to the edge
-      // TODO: if an existing edge was clobbered, perhaps we need to
-      // update anything that named it
-      sm.putNamedEdge(edgeName, newEdge);
-      return true;
+      return reattachEdge(sg, sm, edge, edgeName, gov, dep);
     }
     return false;
   }
diff --git a/test/src/edu/stanford/nlp/semgraph/semgrex/ssurgeon/SsurgeonTest.java b/test/src/edu/stanford/nlp/semgraph/semgrex/ssurgeon/SsurgeonTest.java
@@ -1271,6 +1271,59 @@ public void readXMLMergeNodesAttributes() {
     assertEquals("bar", prof.lemma());
   }
 
+  /**
+   * This test checks some results when there are two candidate head nodes
+   */
+  @Test
+  public void readXMLMergeNodesTwoHeads() {
+    Ssurgeon inst = Ssurgeon.inst();
+
+    String merge = String.join(newline,
+                               "<ssurgeon-pattern-list>",
+                               "  <ssurgeon-pattern>",
+                               "    <uid>38</uid>",
+                               "    <notes>Merge two nodes that should not have been split</notes>",
+                               "    <semgrex>" + XMLUtils.escapeXML("{word:foo}=first . {word:bar}=second") + "</semgrex>",
+                               "    <edit-list>mergeNodes -node first -node second</edit-list>",
+                               "  </ssurgeon-pattern>",
+                               "</ssurgeon-pattern-list>");
+    List<SsurgeonPattern> patterns = inst.readFromString(merge);
+    assertEquals(patterns.size(), 1);
+    SsurgeonPattern mergeSsurgeon = patterns.get(0);
+
+    SemanticGraph sg = SemanticGraph.valueOf("[test-1 obj> foo-2 obj> bar-3]");
+    SemanticGraph newSG = mergeSsurgeon.iterate(sg).first;
+    SemanticGraph expected = SemanticGraph.valueOf("[test-1 obj> foobar-2]");
+    assertEquals(expected, newSG);
+
+    // this one should not have been edited, since the relations are different
+    sg = SemanticGraph.valueOf("[test-1 obj> foo-2 nsubj> bar-3]");
+    newSG = mergeSsurgeon.iterate(sg).first;
+    assertEquals(sg, newSG);
+
+    // this one should not have been edited, since the heads are different
+    sg = SemanticGraph.valueOf("[test-1 obj> [a-2 det> foo-4] nsubj> [b-3 det> bar-5]]");
+    newSG = mergeSsurgeon.iterate(sg).first;
+    assertEquals(sg, newSG);
+
+    // various configurations of two nodes with the same head and different children
+    // the children in these configurations should all be attached to the new merged node
+    sg = SemanticGraph.valueOf("[test-1 obj> [foo-2 nsubj> baz-4] obj> bar-3]");
+    newSG = mergeSsurgeon.iterate(sg).first;
+    expected = SemanticGraph.valueOf("[test-1 obj> [foobar-2 nsubj> baz-3]]");
+    assertEquals(expected, newSG);
+
+    sg = SemanticGraph.valueOf("[test-1 obj> [foo-2 nsubj> baz-4] obj> [bar-3 det> the-5]]");
+    newSG = mergeSsurgeon.iterate(sg).first;
+    expected = SemanticGraph.valueOf("[test-1 obj> [foobar-2 nsubj> baz-3 det> the-4]]");
+    assertEquals(expected, newSG);
+
+    sg = SemanticGraph.valueOf("[test-1 obj> foo-2 obj> [bar-3 det> the-4]]");
+    newSG = mergeSsurgeon.iterate(sg).first;
+    expected = SemanticGraph.valueOf("[test-1 obj> [foobar-2 det> the-3]]");
+    assertEquals(expected, newSG);
+  }
+
   /**
    * Test a basic case of two nodes that should be merged
    *<br>
