Add coref_resolved method to CorefPredictor

[shellshock1911]

• Resolves coreferences by producing a document that has had
  its coreferences substituted with their main mentions

• Ex:
  Personal

  "Charlie wants to buy a game, so he can play it with friends."
  -->
  "Charlie wants to buy a game, so Charlie can play a game
  with friends."

  Possessive

  "Stocks also got a boost after China took steps to encourage
  bank lending and stimulate the country's flagging economy."
  -->
  "Stocks also got a boost after China took steps to encourage
  bank lending and stimulate China's flagging economy."

[shellshock1911]

Addresses #2181 @schmmd

[schmmd]

What is this doing exactly?

[shellshock1911]

Since resolved is a list of the tokens in the document with white space, line 96 replaces the first coreference token with the mention it refers to, while lines 97-98 goes through and masks out all subsequent tokens with "", so that they are ultimately eliminated in the "".join that follows. This procedure is necessary for replacing multi-word coreferences with a single mention, e.g. "the country" with "China" in example. This logic is borrowed from lines 262-271 here.

[schmmd]

Ah--that makes sense. I recommend turning part of your explanation into a comment.

[shellshock1911]

Sounds good, yea I'll add that comment. resolved.remove(i) would work as well, but it's slightly more inefficient (quadratic vs. linear).

[schmmd]

Perhaps Spacy avoids reconstructing a sentence because the possessive suffix depends on the resolved word. E.g. "Michael's" (singluar) but "zebras'" (plural). Your logic seems like a reasonable compromise, but I would expect a clear disclaimer.

[shellshock1911]

Yes, that is one area where mistakes in grammar might occur, in producing "zebras's" instead of "zebras'". In the spaCy neuralcoref extension, you're right that it doesn't add this reconstruction, and instead replaces possessive pronouns with their main mention directly, e.g. "the country's" becomes "China". I can see how certain applications would benefit from maintaining the possessive form, while others would do fine without it. More if..else style code could be added to guarantee grammatical accuracy in these cases, but it's probably stretching the logic too far and opening up further sources of error.

[schmmd]

Yep--it would be stretching the logic too far. I recommend just adding a disclaimer about the issue in the comments of this method.

[schmmd]

@shellshock1911 thanks for the examples in the description. I think we should have them as test cases if this is to be merged.

[shellshock1911]

These are the examples I was using for testing, that could be used as test cases here as well:

• "Charlie wants to buy chocolate. He loves it so much. Julie wants to buy fruit. That is what she loves."
• "The woman reading a newspaper sat on the bench with her dog."
• "At the American Economic Association conference in Atlanta Friday morning, Federal Reserve Chairman Jerome Powell reiterated the strengths of the economy but also signaled the Fed would remain flexible in its management of interest rates. The Fed has worked to gradually increase interest rates over the past couple years and signaled that it would hike rates two or three times next year."
• "Stocks also got a boost after China took steps to encourage bank lending and stimulate the country's flagging economy."
• "The People's Bank of China announced it would slash the amount of money that banks are required to hold in reserve, the latest in a series of policy changes the government has taken to support growth."

Testing these shows that the method can accurately handle both multi-word main mentions and coreferences, as well as both personal and possessive pronouns. Mistakes in resolution, such as in the third example, result from errors in the underlying model's prediction, and so are out of the scope of this contribution.

[schmmd]

@shellshock1911 I'd like to merge this but we need two things:

1. The build breaks must be fixed.
2. You should add a unit test for this functionality.

Then we're good to go!

[shellshock1911]

@schmmd - Awesome, I've made changes to fix issues with the linting and types, but I kept failing my unit tests in the build system, although they pass locally. I looked into more and realized that the serialized models provided in allennlp/tests/fixtures are much smaller (52K compressed) than the ones in the online docs (57M), therefore their behavior is different on the same inputs. Are these models meant to demonstrate the expected interface of a particular model, without any accuracy guarantees? If that's the case, then should the unit tests check the type and structure of a models output, but not check for correctness? Thanks.

[schmmd]

@shellshock1911 it looks like there's still a linting issue.

For your test case, I probably wouldn't call a predictor in the test case. I would probably have only one or two examples and provide hard-coded predictions as inputs. The test would then check if resolving the example gives the text you intended.

That way your test will run faster and will be independent of the actual coreference models.

[schmmd]

@shellshock1911 to more directly answer your question--yes the test fixture models are used to test interfaces and not correctness. But for your unit test, I would avoid using any models and instead hard-code predictions.

[shellshock1911]

Thanks for approving this PR!

[schmmd]

Thanks for the hard work! I'm not going to merge it right now as I'm just about to head out for a week vacation--but someone else may or I will as soon as I'm back!

[shellshock1911]

Any thoughts on this? Changes in the upstream introduced a linting issue, but I resolved that and now the build is good 😄.

[matt-gardner]

@schmmd, looks like you forgot about this - feel free to be brave and click merge if the tests pass =).

[shellshock1911]

Thanks guys! I do still think this would be a useful utility to the coreference predictor. In recent weeks, I've been running news article processing pipeline with this fork of AllenNLP, where this in-place resolution method has been applied on millions of articles without apparent error, i.e. no crashes. It'd be awesome to have though in the upstream and I'm sure others could benefit as well since it automates the coreference resolution step of pre-processing in a larger NLP pipeline.

[matt-gardner]

Thanks @shellshock1911! Sorry this fell through the cracks and took so long to get merged.