-
Notifications
You must be signed in to change notification settings - Fork 7
feat(medcat-service): Medcat Demo and AnonCAT demo uplift in Gradio #279
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
base: main
Are you sure you want to change the base?
Conversation
…nd improve test coverage in test_demo_logic.py
…entity resolution
mart-r
left a comment
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.
Overall I think this looks great!
Left a few comments on type hints for return types.
And one on the long-winded setup for the demo web interface that I think could use some refactoring for better readability and to reduce nesting.
| ] | ||
|
|
||
|
|
||
| def perform_named_entity_resolution(input_text: str, redact: bool | None = None): |
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.
Perhaps adding the return type a type hint here would be useful? You've descried it in the docs anyway (though it's missing the last str).
I.e -> tuple[dict, list[list[str]], str]
| return response_tuple | ||
|
|
||
|
|
||
| def medcat_demo_perform_named_entity_resolution(input_text: str): |
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.
Again, including the return type (-> tuple[dict, list[list[str]]]) could be useful.
| return result[0], result[1] | ||
|
|
||
|
|
||
| def anoncat_demo_perform_deidentification(input_text: str, redact: bool): |
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.
Again, return type might be useful.
| return annotation_details_placeholder_text | ||
|
|
||
|
|
||
| if settings.deid_mode: |
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 feels a little off to me. I suppose global state is to be expected here, but I wouldn't expect as much code to exist just within the module scope.
Both these (if / else) follow a pretty similar pattern (gr.Block -> gr.Row -> gr.Column), perhaps there's a way to simplify these to avoid some of the duplication?
But perhaps I'd just feel better if the logic (i.e the DeID mode and the regular NER+L mode) were just in their own separate methods that were called here.
The other thing to note is that there's quite a bit of nesting here (6 layers deep). Perhaps we can mitigate that a little? Perhaps we could split out some parts to separate methods?
Screenshots
Before
After
After: Anoncat demo
Split into two tabs to show details. First tab should be at least as good as existing one