-
-
Notifications
You must be signed in to change notification settings - Fork 94
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.
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
Add entity linking task #241
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be great to have :-)
I have yet to play around with it more and I haven't reviewed the unit tests yet, but I figured I'd already send my first batch of review comments.
# Conflicts: # .github/workflows/test_gpu.yml # spacy_llm/tasks/__init__.py # spacy_llm/tasks/builtin_task.py # spacy_llm/tasks/lemma/registry.py # spacy_llm/tasks/lemma/util.py # spacy_llm/tasks/ner/registry.py # spacy_llm/tasks/ner/util.py # spacy_llm/tasks/rel/registry.py # spacy_llm/tasks/rel/util.py # spacy_llm/tasks/sentiment/registry.py # spacy_llm/tasks/sentiment/util.py # spacy_llm/tasks/spancat/registry.py # spacy_llm/tasks/spancat/util.py # spacy_llm/tasks/summarization/registry.py # spacy_llm/tasks/summarization/util.py # spacy_llm/tasks/textcat/registry.py # spacy_llm/tasks/textcat/util.py # spacy_llm/ty.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the recent refactors have really improved the usability of this feature :-)
I'm not done reviewing yet, but need to break here for a bit, so sending my first batch of comments/suggestions.
Co-authored-by: Sofie Van Landeghem <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking really good! This now works nicely with the NIL option, and the unit tests look great. I was able to get it to output different identifiers for the city of New York, the state, or using the string "New York" in a totally different context - it outputted NIL in that case. Nice!
Comments are only nitpicks.
One final issue to discuss is how to best support in-code experimenting with nlp.add_pipe("llm_entitylinker")
and to add a unit test for that, too.
Co-authored-by: Sofie Van Landeghem <[email protected]>
Co-authored-by: Sofie Van Landeghem <[email protected]>
Co-authored-by: Sofie Van Landeghem <[email protected]>
Co-authored-by: Sofie Van Landeghem <[email protected]>
Added a unit test in a631278. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Let's get this merged 🎉
Description
Add entity linking task.
Corresponding documentation PR
explosion/spaCy#12988
Types of change
Checklist
tests
andusage_examples/tests
, and all new and existing tests passed. This includespytest
ran with--external
)pytest
ran with--gpu
)