Fix issue 2396

[alvaroabascar]

Description

This PR fixes issue #2396: Doc.get_lca_matrix had a bug in this line which caused wrong outputs
for some pairs of tokens.

Types of change

Bug fix

Explanation of the bug

I borrow the following example from @slavi (#2396):

s = "He created a test for spacy"
doc = nlp(s)
doc.get_lca_matrix()[3][5]

• Expected output: 3
• Actual output: 1

The above mentioned line is:

...
            else:
                lca_index = __pairwise_lca(token_j.head, token_k.head, lca_matrix)
...

Here, for tokens 3 ("test") and 5 ("spacy"), we are calling __pairwise_lca recursively with their heads ("created" and "for" respectively). But in doing so, we are skiping "test", which is the actual lowest common ancestor.

Solution

The reimplementation consists on this simple algorihtm. It works as expected and is 3 times faster.

Checklist

• I have submitted the spaCy Contributor Agreement.
• I ran the tests, and all new and existing tests passed.
• My changes don't require a change to the documentation, or if they do, I've added all required information.

[honnibal]

Really nice first PR, thanks!

I noticed some ugliness with the original code as well --- unfortunately the implementation appears to be duplicated in the spacy/tokens/span.pyx module. It would probably be good to move the code in spacy/tokens/doc.pyx into functions so that span.pyx can call into it. If that ends up being tricky, we can also just punt on that and just apply the same fix to the span.pyx code, and let the implementations continue in parallel for now.

[alvaroabascar]

True! I hadn't seen the duplicated implementation inside span.pyx. I can either:

a) Separate the function and call it from both Span.get_lca_matrix and Doc.get_lca_matrix.
b) Implement the code in Span.get_lca_matrix, and from Doc.get_lca_matrix just call self[:].get_lca_matrix().

b) seems cleaner to me. But in case you'd prefer a), where would you place this auxiliary function?

[honnibal]

Delegating to a span of the whole document is a clever solution, but I think I probably prefer a helper function in doc.pyx. I probably wouldn't expect to find the implementation within span.pyx, since the Span is a view of the doc, so normally the delegation goes the other way.

Moving this out into a helper function also lets us fix that closure, which I always found a bit ugly about the previous implementation. So, we want the function to be something like:

def _get_lca_matrix(Doc doc, int start, int end):

We should keep it as a private function so that we don't have to worry about validating the start and end offsets within it. From the span, if we call _get_lca_matrix(self.doc, self.start, self.end) we know the start and end are valid.

[alvaroabascar]

Done! I first tried to just define a helper _get_lca_matrix inside doc.pyx, but could not import it from span.pyx. It finally worked when I declared it in doc.pxd. I'm not experienced in Cython so I hope the approach is right.

I also removed that closure within _get_lca_matrix, and I defined it as another helper _get_tokens_lca, which made me wonder whether that helper should go in doc.pyx or token.pyx.

Finally, I guess that both helpers could be further "cythonized" resorting to TokenC, but this would mean renouncing to token.ancestors, which makes everything pretty clean.

Let me know what you think :)

[honnibal]

Looks great, thanks! I think keeping that helper in the doc.pyx is fine. It's nice to have the helper close to where it's used. I think you're right that keeping it as Python so we can use the ancestors property is better.

[honnibal]

Hm, the span test is failing, though. Is the test wrong or the code?

[alvaroabascar]

It was a mistake in the code, but it should be fine now.

[honnibal]

Sweet, merging.