Fix issue 2396 (#3089)

* Test on #2396: bug in Doc.get_lca_matrix()

* reimplementation of Doc.get_lca_matrix(), (closes #2396)

* reimplement Span.get_lca_matrix(), and call it from Doc.get_lca_matrix()

* tests Span.get_lca_matrix() as well as Doc.get_lca_matrix()

* implement _get_lca_matrix as a helper function in doc.pyx; call it from Doc.get_lca_matrix and Span.get_lca_matrix

* use memory view instead of np.ndarray in _get_lca_matrix (faster)

* fix bug when calling Span.get_lca_matrix; return lca matrix as np.array instead of memoryview

* cleaner conditional, add comment

.github/contributors/alvaroabascar.md

@@ -0,0 +1,106 @@
+# spaCy contributor agreement
+
+This spaCy Contributor Agreement (**"SCA"**) is based on the
+[Oracle Contributor Agreement](http://www.oracle.com/technetwork/oca-405177.pdf).
+The SCA applies to any contribution that you make to any product or project
+managed by us (the **"project"**), and sets out the intellectual property rights
+you grant to us in the contributed materials. The term **"us"** shall mean
+[ExplosionAI UG (haftungsbeschränkt)](https://explosion.ai/legal). The term
+**"you"** shall mean the person or entity identified below.
+
+If you agree to be bound by these terms, fill in the information requested
+below and include the filled-in version with your first pull request, under the
+folder [`.github/contributors/`](/.github/contributors/). The name of the file
+should be your GitHub username, with the extension `.md`. For example, the user
+example_user would create the file `.github/contributors/example_user.md`.
+
+Read this agreement carefully before signing. These terms and conditions
+constitute a binding legal agreement.
+
+## Contributor Agreement
+
+1. The term "contribution" or "contributed materials" means any source code,
+object code, patch, tool, sample, graphic, specification, manual,
+documentation, or any other material posted or submitted by you to the project.
+
+2. With respect to any worldwide copyrights, or copyright applications and
+registrations, in your contribution:
+
+    * you hereby assign to us joint ownership, and to the extent that such
+    assignment is or becomes invalid, ineffective or unenforceable, you hereby
+    grant to us a perpetual, irrevocable, non-exclusive, worldwide, no-charge,
+    royalty-free, unrestricted license to exercise all rights under those
+    copyrights. This includes, at our option, the right to sublicense these same
+    rights to third parties through multiple levels of sublicensees or other
+    licensing arrangements;
+
+    * you agree that each of us can do all things in relation to your
+    contribution as if each of us were the sole owners, and if one of us makes
+    a derivative work of your contribution, the one who makes the derivative
+    work (or has it made will be the sole owner of that derivative work;
+
+    * you agree that you will not assert any moral rights in your contribution
+    against us, our licensees or transferees;
+
+    * you agree that we may register a copyright in your contribution and
+    exercise all ownership rights associated with it; and
+
+    * you agree that neither of us has any duty to consult with, obtain the
+    consent of, pay or render an accounting to the other for any use or
+    distribution of your contribution.
+
+3. With respect to any patents you own, or that you can license without payment
+to any third party, you hereby grant to us a perpetual, irrevocable,
+non-exclusive, worldwide, no-charge, royalty-free license to:
+
+    * make, have made, use, sell, offer to sell, import, and otherwise transfer
+    your contribution in whole or in part, alone or in combination with or
+    included in any product, work or materials arising out of the project to
+    which your contribution was submitted, and
+
+    * at our option, to sublicense these same rights to third parties through
+    multiple levels of sublicensees or other licensing arrangements.
+
+4. Except as set out above, you keep all right, title, and interest in your
+contribution. The rights that you grant to us under these terms are effective
+on the date you first submitted a contribution to us, even if your submission
+took place before the date you sign these terms.
+
+5. You covenant, represent, warrant and agree that:
+
+    * Each contribution that you submit is and shall be an original work of
+    authorship and you can legally grant the rights set out in this SCA;
+
+    * to the best of your knowledge, each contribution will not violate any
+    third party's copyrights, trademarks, patents, or other intellectual
+    property rights; and
+
+    * each contribution shall be in compliance with U.S. export control laws and
+    other applicable export and import laws. You agree to notify us if you
+    become aware of any circumstance which would make any of the foregoing
+    representations inaccurate in any respect. We may publicly disclose your
+    participation in the project, including the fact that you have signed the SCA.
+
+6. This SCA is governed by the laws of the State of California and applicable
+U.S. Federal law. Any choice of law rules will not apply.
+
+7. Please place an “x” on one of the applicable statement below. Please do NOT
+mark both statements:
+
+    * [x] I am signing on behalf of myself as an individual and no other person
+    or entity, including my employer, has or will have rights with respect to my
+    contributions.
+
+    * [ ] I am signing on behalf of my employer or a legal entity and I have the
+    actual authority to contractually bind that entity.
+
+## Contributor Details
+
+| Field                          | Entry                |
+|------------------------------- | -------------------- |
+| Name                           |     Álvaro Abella    |
+| Company name (if applicable)   |         IOMED        |
+| Title or role (if applicable)  |          CSO         |
+| Date                           |       21/12/2018     |
+| GitHub username                |      alvaroabascar   |
+| Website (optional)             |                      |

spacy/tests/regression/test_issue2396.py

@@ -0,0 +1,27 @@
+# coding: utf-8
+from __future__ import unicode_literals
+
+from ..util import get_doc
+
+import pytest
+import numpy
+
+@pytest.mark.parametrize('sentence,matrix', [
+    (
+        'She created a test for spacy',
+        numpy.array([
+            [0, 1, 1, 1, 1, 1],
+            [1, 1, 1, 1, 1, 1],
+            [1, 1, 2, 3, 3, 3],
+            [1, 1, 3, 3, 3, 3],
+            [1, 1, 3, 3, 4, 4],
+            [1, 1, 3, 3, 4, 5]], dtype=numpy.int32)
+    )
+    ])
+def test_issue2396(EN, sentence, matrix):
+    doc = EN(sentence)
+    span = doc[:]
+    assert (doc.get_lca_matrix() == matrix).all()
+    assert (span.get_lca_matrix() == matrix).all()
+
+

spacy/tokens/doc.pxd

@@ -30,6 +30,9 @@ cdef int token_by_end(const TokenC* tokens, int length, int end_char) except -2
 
 cdef int set_children_from_heads(TokenC* tokens, int length) except -1
 
+
+cdef int [:,:] _get_lca_matrix(Doc, int start, int end)
+
 cdef class Doc:
     cdef readonly Pool mem
     cdef readonly Vocab vocab

spacy/tokens/doc.pyx

@@ -1,3 +1,4 @@
+
 # coding: utf8
 # cython: infer_types=True
 # cython: bounds_check=False
@@ -715,48 +716,14 @@ cdef class Doc:
         return self
 
     def get_lca_matrix(self):
-        """Calculates the lowest common ancestor matrix for a given `Doc`.
-        Returns LCA matrix containing the integer index of the ancestor, or -1
-        if no common ancestor is found (ex if span excludes a necessary
-        ancestor). Apologies about the recursion, but the impact on
-        performance is negligible given the natural limitations on the depth
-        of a typical human sentence.
+        """Calculates a matrix of Lowest Common Ancestors (LCA) for a given
+        `Doc`, where LCA[i, j] is the index of the lowest common ancestor among
+        token i and j.
+
+        RETURNS (np.array[ndim=2, dtype=numpy.int32]): LCA matrix with shape
+            (n, n), where n = len(self).
         """
-        # Efficiency notes:
-        # We can easily improve the performance here by iterating in Cython.
-        # To loop over the tokens in Cython, the easiest way is:
-        # for token in doc.c[:doc.c.length]:
-        #     head = token + token.head
-        # Both token and head will be TokenC* here. The token.head attribute
-        # is an integer offset.
-        def __pairwise_lca(token_j, token_k, lca_matrix):
-            if lca_matrix[token_j.i][token_k.i] != -2:
-                return lca_matrix[token_j.i][token_k.i]
-            elif token_j == token_k:
-                lca_index = token_j.i
-            elif token_k.head == token_j:
-                lca_index = token_j.i
-            elif token_j.head == token_k:
-                lca_index = token_k.i
-            elif (token_j.head == token_j) and (token_k.head == token_k):
-                lca_index = -1
-            else:
-                lca_index = __pairwise_lca(token_j.head, token_k.head,
-                                           lca_matrix)
-            lca_matrix[token_j.i][token_k.i] = lca_index
-            lca_matrix[token_k.i][token_j.i] = lca_index
-
-            return lca_index
-
-        lca_matrix = numpy.empty((len(self), len(self)), dtype=numpy.int32)
-        lca_matrix.fill(-2)
-        for j in range(len(self)):
-            token_j = self[j]
-            for k in range(j, len(self)):
-                token_k = self[k]
-                lca_matrix[j][k] = __pairwise_lca(token_j, token_k, lca_matrix)
-                lca_matrix[k][j] = lca_matrix[j][k]
-        return lca_matrix
+        return numpy.asarray(_get_lca_matrix(self, 0, len(self)))
 
     def to_disk(self, path, **exclude):
         """Save the current state to a directory.
@@ -1060,6 +1027,73 @@ cdef int set_children_from_heads(TokenC* tokens, int length) except -1:
             tokens[tokens[i].l_edge].sent_start = True
 
 
+cdef int _get_tokens_lca(Token token_j, Token token_k):
+    """Given two tokens, returns the index of the lowest common ancestor
+    (LCA) among the two. If they have no common ancestor, -1 is returned.
+
+    token_j (Token): a token.
+    token_k (Token): another token.
+    RETURNS (int): index of lowest common ancestor, or -1 if the tokens
+        have no common ancestor.
+    """
+    if token_j == token_k:
+        return token_j.i
+    elif token_j.head == token_k:
+        return token_k.i
+    elif token_k.head == token_j:
+        return token_j.i
+
+    token_j_ancestors = set(token_j.ancestors)
+
+    if token_k in token_j_ancestors:
+        return token_k.i
+
+    for token_k_ancestor in token_k.ancestors:
+
+        if token_k_ancestor == token_j:
+            return token_j.i
+
+        if token_k_ancestor in token_j_ancestors:
+            return token_k_ancestor.i
+
+    return -1
+
+
+cdef int [:,:] _get_lca_matrix(Doc doc, int start, int end):
+    """Given a doc and a start and end position defining a set of contiguous
+    tokens within it, returns a matrix of Lowest Common Ancestors (LCA), where
+    LCA[i, j] is the index of the lowest common ancestor among token i and j.
+    If the tokens have no common ancestor within the specified span,
+    LCA[i, j] will be -1.
+
+    doc (Doc): The index of the token, or the slice of the document
+    start (int): First token to be included in the LCA matrix.
+    end (int): Position of next to last token included in the LCA matrix.
+    RETURNS (int [:, :]): memoryview of numpy.array[ndim=2, dtype=numpy.int32],
+        with shape (n, n), where n = len(doc).
+    """
+    cdef int [:,:] lca_matrix
+
+    n_tokens= end - start
+    lca_matrix = numpy.empty((n_tokens, n_tokens), dtype=numpy.int32)
+
+    for j in range(start, end):
+        token_j = doc[j]
+        # the common ancestor of token and itself is itself:
+        lca_matrix[j, j] = j
+        for k in range(j + 1, end):
+            lca = _get_tokens_lca(token_j, doc[k])
+            # if lca is outside of span, we set it to -1
+            if not start <= lca < end:
+                lca_matrix[j, k] = -1
+                lca_matrix[k, j] = -1
+            else:
+                lca_matrix[j, k] = lca
+                lca_matrix[k, j] = lca
+
+    return lca_matrix
+
+
 def pickle_doc(doc):
     bytes_data = doc.to_bytes(vocab=False, user_data=False)
     hooks_and_data = (doc.user_data, doc.user_hooks, doc.user_span_hooks,

spacy/tokens/span.pyx

@@ -7,7 +7,8 @@ import numpy
 import numpy.linalg
 from libc.math cimport sqrt
 
-from .doc cimport token_by_start, token_by_end, get_token_attr
+from .doc cimport token_by_start, token_by_end, get_token_attr, _get_lca_matrix
+from .token cimport TokenC
 from ..structs cimport TokenC, LexemeC
 from ..typedefs cimport flags_t, attr_t, hash_t
 from ..attrs cimport attr_id_t
@@ -183,6 +184,17 @@ cdef class Span:
         return self.doc.merge(self.start_char, self.end_char, *args,
                               **attributes)
 
+    def get_lca_matrix(self):
+        """Calculates a matrix of Lowest Common Ancestors (LCA) for a given
+        `Span`, where LCA[i, j] is the index of the lowest common ancestor among
+        the tokens span[i] and span[j]. If they have no common ancestor within
+        the span, LCA[i, j] will be -1.
+
+        RETURNS (np.array[ndim=2, dtype=numpy.int32]): LCA matrix with shape
+            (n, n), where n = len(self).
+        """
+        return numpy.asarray(_get_lca_matrix(self.doc, self.start, self.end))
+
     def similarity(self, other):
         """Make a semantic similarity estimate. The default estimate is cosine
         similarity using an average of word vectors.
@@ -209,47 +221,6 @@ cdef class Span:
             return 0.0
         return numpy.dot(self.vector, other.vector) / (self.vector_norm * other.vector_norm)
 
-    def get_lca_matrix(self):
-        """Calculates the lowest common ancestor matrix for a given `Span`.
-        Returns LCA matrix containing the integer index of the ancestor, or -1
-        if no common ancestor is found (ex if span excludes a necessary
-        ancestor). Apologies about the recursion, but the impact on
-        performance is negligible given the natural limitations on the depth
-        of a typical human sentence.
-        """
-        def __pairwise_lca(token_j, token_k, lca_matrix, margins):
-            offset = margins[0]
-            token_k_head = token_k.head if token_k.head.i in range(*margins) else token_k
-            token_j_head = token_j.head if token_j.head.i in range(*margins) else token_j
-            token_j_i = token_j.i - offset
-            token_k_i = token_k.i - offset
-            if lca_matrix[token_j_i][token_k_i] != -2:
-                return lca_matrix[token_j_i][token_k_i]
-            elif token_j == token_k:
-                lca_index = token_j_i
-            elif token_k_head == token_j:
-                lca_index = token_j_i
-            elif token_j_head == token_k:
-                lca_index = token_k_i
-            elif (token_j_head == token_j) and (token_k_head == token_k):
-                lca_index = -1
-            else:
-                lca_index = __pairwise_lca(token_j_head, token_k_head, lca_matrix, margins)
-            lca_matrix[token_j_i][token_k_i] = lca_index
-            lca_matrix[token_k_i][token_j_i] = lca_index
-            return lca_index
-
-        lca_matrix = numpy.empty((len(self), len(self)), dtype=numpy.int32)
-        lca_matrix.fill(-2)
-        margins = [self.start, self.end]
-        for j in range(len(self)):
-            token_j = self[j]
-            for k in range(len(self)):
-                token_k = self[k]
-                lca_matrix[j][k] = __pairwise_lca(token_j, token_k, lca_matrix, margins)
-                lca_matrix[k][j] = lca_matrix[j][k]
-        return lca_matrix
-
     cpdef np.ndarray to_array(self, object py_attr_ids):
         """Given a list of M attribute IDs, export the tokens to a numpy
         `ndarray` of shape `(N, M)`, where `N` is the length of the document.