-
Notifications
You must be signed in to change notification settings - Fork 14
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
Remove Spacy hard dependency on forte medical ontology #96
Remove Spacy hard dependency on forte medical ontology #96
Conversation
|
||
entity.umls_entities.append(umls) | ||
setattr(entity, "umls_entities", umls_entries) |
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 we cannot do setattr
here since the list in forte entries are not normal python lists (https://github.com/asyml/forte/blob/master/ftx/onto/clinical.py#L69). This is because we actually need to store reference instead of the actual content. So we should probably do something like getattr(entity, "umls_entities").append(umls_entry))
(Need to try it out to see if it works).
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 also realize the test case didn't test this part: https://github.com/asyml/forte-wrappers/blob/main/tests/wrappers/spacy_processors_test.py, maybe we should add a small test here?
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 have tested the current code, and it does work as expected. I see the umls_entities
list populated properly. Maybe because Flist
and native lists, both are mutable sequences, we are able to append to Flist
through setattr too. I can try using and testing getattr(entity, "umls_entities").append(umls_entry))
as well.
I will add the missing test case for this use case.
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.
Added "umls_link" working to the existing test cases. Other tests run within seconds, but this one goes on for a very long time on my local machine. And I don't think tests are enabled in forte-wrapper's CI pipeline?
…//github.com/Piyush13y/forte-wrappers into 96_remove_spacy_dependency_on_forte_med_onto
…//github.com/Piyush13y/forte-wrappers into 96_remove_spacy_dependency_on_forte_med_onto
This PR fixes #95.
Description of changes
SpacyProcessor
now can be run with theumls_link
processor config, without a hard dependency on Forte.The future scope is to have all medical related ontologies in the Forte-medicine repo, so the default config has been set as so. However, until we reach that point, one can run
SpacyProcessor
withumls_link
config in Forte too, using customized ontology types by passing them as config.Possible influences of this PR.
No side-effects, ensures consistency and disambiguity in the long run.