marcotcr / checklist

Beyond Accuracy: Behavioral Testing of NLP models with CheckList
MIT License
2.01k stars 204 forks source link

Fix bug with add_negation and do+verb #43

Closed kevinrobinson closed 3 years ago

kevinrobinson commented 4 years ago

hello! Thanks for sharing this work in the open 👍

I ran into a bug / unfinished part of the library. Here's the test case and a colab:

import spacy
import checklist
from checklist.perturb import Perturb

text = "they know the answer"
nlp = spacy.load('en_core_web_sm')
pdata = list(nlp.pipe([text]))
Perturb.add_negation(pdata[0])

This runs into:

---------------------------------------------------------------------------

TypeError                                 Traceback (most recent call last)

<ipython-input-229-e455e75abd10> in <module>()
----> 1 Perturb.add_negation(pdata[0])

/usr/local/lib/python3.6/dist-packages/checklist/perturb.py in add_negation(doc)
    295                     # params = [tense, 3]
    296                     if root.tag_ not in ['VBG']:
--> 297                         do = pattern.en.conjugate('do', *params) + 'n\'t'
    298                         new_root = pattern.en.conjugate(root.text, tense='infinitive')
    299                     else:

TypeError: unsupported operand type(s) for +: 'NoneType' and 'str'

The immediate issue is that the first tense for know is ('infinitive', None, None, None, None), and when this code tries to splice in the present tense, which is most common, it tries to call:

pattern.en.conjugate('do', *['present', None, None, None, None]) + 'n\'t'

which is what raises.

I don't entirely understand what this code what trying to do, but I'm guessing it was trying to take the first tense that matched the most common verb tense. I also noticed the TODO comment a few lines above, and the commented out code a few lines below made me think that maybe this branch just wasn't finished :) I looked for test cases for this function but didn't find any, so I just verified that this patch fixes the bug here. But you might want to check if there are other cases this would break, it's hard for me to verify that. Thanks! :)

marcotcr commented 3 years ago

Thanks for spotting the bug. The fix was a bit more involved, but I think I fixed it in 53d3671