Fix the EvaluationStore to match prediction and target entities

[davidezanella]

Proposed changes:

• Fix the EvaluationStore serialise method: the prediction and target entities were not aligned. Previously, they were aligned only by adding 'None' at the end of the smaller list instead of looking where they should be placed.

Practical example:

target entities: [
   {'text': 'hi, how are you', 'start': 0, 'end': 2, entity: 'verb' },
   {'text': 'hi, how are you', 'start': 4, 'end': 7, entity: 'noun' }
]
predicted entities: [
   {'text': 'hi, how are you', 'start': 4, 'end': 7, entity: 'noun' }
]

Adding 'None' at the end, as done originally, leads to wrong metrics because even the right prediction was marked as wrong.

Status (please check what you already did):

• added some tests for the functionality
• updated the documentation
• updated the changelog (please check changelog for instructions)
• reformat files using black (please check Readme for instructions)

[sara-tagger]

Thanks for submitting a pull request 🚀 @degiz will take a look at it as soon as possible ✨

[davidezanella]

@tabergma can you review this pr?

[degiz]

Hey @davidezanella

Thanks a lot for the PR! Could you please also write a unit for the change? I believe you can use existing tests/test_test.py file.

[tabergma]

@davidezanella Thanks for addressing this. I think your concern is valid and should be fixed, but I am not sure if your method fixes the complete problem. Currently you just iterating over the gold entities to see if there are matching predicted entities and you are adding None in case no matching entity could be found. That way we ensure that the size of the gold end predicted entities is the same. However, i think we also need to consider the case when we have more predicted entities than gold entities. For example, you have a sentence like "Tanja is currently in Munich, but she lives in Berlin" and the annotator made a mistake and just annotate "Tanja" as person, but forgot to tag "Munich" and "Berlin" as city. However, the model predicts all three of them. If I understand your code correctly, you would just consider the gold entity "Tanja" and you would ignore the two additional entities "Munich" and "Berlin", which is not correct as those are false positive predictions in this case. What do you think?

[davidezanella]

Hey @tabergma, you are right! I've fixed the code and the tests.
Let me know what do you think about it

[degiz]

doesn't seem I can cancel my "changes requested" 😞
@tabergma will review the code.

[tabergma]

@davidezanella Sorry for the late reply, I was offline a couple of days. Will take a look at the PR today. Can you please resolve the conflicts in the meantime? Thanks.

[tabergma]

Looks great 🚀 Added a few comments. Can you please also add a changelog entry? Thanks.

[tabergma]

Looks great 💯 Thanks for fixing this!

[tabergma]

Minor: We try to not use abbreviations in names. I would rename this to index_prediction and index_target.

[tabergma]

Minor: I would rename this to comparison_result.

[tabergma]

Minor: Rename to index_prediction.

[tabergma]

Minor: Rename to index_target.

[tabergma]

@degiz I think you need to update your review otherwise we are not able to merge this PR.