Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix EL failure with sentence-crossing entities #12398

Merged
merged 16 commits into from
Mar 14, 2023
Merged

Fix EL failure with sentence-crossing entities #12398

merged 16 commits into from
Mar 14, 2023

Conversation

rmitsch
Copy link
Contributor

@rmitsch rmitsch commented Mar 10, 2023 •
edited
Loading

Description

Types of change

EntityLinker crashes when incl_context is True and len(list(ent.sents)) > 1and the first of the sentences inent.sents` (which is the one used in the context calculation) doesn't have an entity.

This currently adds a test to reproduce this behavior. A fix will be added later - probably in this PR, hence this is marked as draft.

Checklist

  • I confirm that I have the right to submit this contribution under the project's MIT license.
  • 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.

@rmitsch rmitsch added bug Bugs and behaviour differing from documentation feat / nel Feature: Named Entity linking labels Mar 10, 2023
@rmitsch rmitsch self-assigned this Mar 10, 2023
@rmitsch
Copy link
Contributor Author

rmitsch commented Mar 14, 2023 •
edited
Loading

Simplified the ent.sents handling in 9b6f47f and fc0f954 as Span.sents has been fixed and doesn't produce partial sentences anymore.

Also: are we ok with this fix or should I remove it for now?

@rmitsch rmitsch changed the title Add test reproducing EL failure in sentence-crossing entities Fix EL failure with sentence-crossing entities Mar 14, 2023
@rmitsch rmitsch marked this pull request as ready for review March 14, 2023 13:10
svlandeg
svlandeg approved these changes Mar 14, 2023
View reviewed changes
Copy link
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That all looks good!

@rmitsch rmitsch merged commit 96b61d0 into explosion:master Mar 14, 2023
rmitsch added a commit to rmitsch/spaCy that referenced this pull request Mar 14, 2023
@rmitsch
Fix EL incl_context bug as in explosion#12398.
7851f6e
adrianeboyd pushed a commit to adrianeboyd/spaCy that referenced this pull request Apr 3, 2023
@rmitsch @svlandeg @adrianeboyd
Fix EL failure with sentence-crossing entities (explosion#12398)
bd0768c 
* Add test reproducing EL failure in sentence-crossing entities.

* Format.

* Draft fix.

* Format.

* Fix case for len(ent.sents) == 1.

* Format.

* Format.

* Format.

* Fix mypy error.

* Merge EL sentence crossing tests.

* Remove unneeded sentencizer component.

* Fix or ignore mypy issues in test.

* Simplify ent.sents handling.

* Format. Update assert in ent.sents handling.

* Small rewrite

---------

Co-authored-by: Sofie Van Landeghem <svlandeg@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@svlandeg svlandeg svlandeg approved these changes

Assignees

@rmitsch rmitsch

Labels
bug Bugs and behaviour differing from documentation feat / nel Feature: Named Entity linking
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rmitsch @svlandeg