search

clulab / eidos

Machine reading system for World Modelers
Apache License 2.0
36 stars 24 forks source link

updates NegationHandler to fix 'not only' bug #1091

Closed zupon closed 2 years ago

zupon commented 2 years ago

Stops from making a negation attachment based on dependencies or left/right context if

kwalcock commented 2 years ago

This seems like something that could get broken, perhaps because something else stops even calling this code. It would be very reassuring to have a unit test showing it in action and remaining in action. Would it be possible to get one at some level? I'm not sure it would have to affect the finding of Mentions or not.

kwalcock commented 2 years ago

@zupon, thanks for tracking that down so deeply buried in the code!

MihaiSurdeanu commented 2 years ago

Thanks @zupon !

I think this should be reorganized as we discussed today. That is, have a component that recognizes fake negation patterns once, and then check that the negation words are not covered by those patterns. Thanks!

zupon commented 2 years ago

I added a Seq for fakeNegation that covers a lot of the similar phrases to "not only". This gives us a more general solution that we can add to if we find other cases.

There is still a hard case that I haven't resolved yet, whereby we get a negation attachment on flooding-->conflict in something like "Flooding did not cause famine, merely conflict." since the span of the event goes from Flooding all the way to conflict, which includes "not" and the neg dependency. This is something to fix, but it might not be that common.

kwalcock commented 2 years ago

LGTM.

This didn't start just now, so it's a separate issue. The file seems more language dependent than others, with some texts and dependencies hard-coded. In some other pass, this file and maybe some others might tidied up in that respect.

kwalcock commented 2 years ago

@MihaiSurdeanu, at the meeting on Thursday we were wondering whether you would want to leave it like this or have it implemented as Odin rules (https://docs.google.com/document/d/1a1dmCW-haqQh4JJ47Quwvv6U0C49c8Pi6P2hjBGNgIc/edit).

MihaiSurdeanu commented 2 years ago

Given that this is a simply surface pattern, it is fine as is! Ok to merge. Thank you @zupon !