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.

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

Added NegationContextAnalyzer processor and unit tests #7

Merged
merged 35 commits into from
Mar 4, 2022

Conversation

Piyush13y
Copy link
Collaborator

@Piyush13y Piyush13y commented Feb 28, 2022

This PR fixes #3

Description of changes

  • NegationContextAnalyzer processor added
  • Unit tests added for the processor
  • Medical pipeline example updated, by adding NegationContextAnalyzer to the pipeline

Possible influences of this PR.

Demo pipeline now uses the new processor and detects negation contexts for entity mentions.

Expected Output

image

Copy link
Member

@hunterhector hunterhector left a comment

Choose a reason for hiding this comment

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

  1. I guess we want to lint the project with mypy, pylint and black to make it more standard before we merge this.
  2. We will also need to set up test cases (and write test cases), like here.

examples/mimic_iii/medical_pipeline.py Outdated Show resolved Hide resolved
examples/mimic_iii/medical_pipeline.py Outdated Show resolved Hide resolved
examples/mimic_iii/medical_pipeline.py Outdated Show resolved Hide resolved
forte_medical/processors/negation_context_analyzer.py Outdated Show resolved Hide resolved
examples/mimic_iii/medical_pipeline.py Outdated Show resolved Hide resolved
examples/mimic_iii/medical_pipeline.py Outdated Show resolved Hide resolved
examples/mimic_iii/negex_triggers.txt Outdated Show resolved Hide resolved
@tanyuqian
Copy link
Member

  1. I guess we want to lint the project with mypy, pylint and black to make it more standard before we merge this.
  2. We will also need to set up test cases (and write test cases), like here.

Sure. I'll update the workflow with more linting tools.

@Piyush13y
Copy link
Collaborator Author

Piyush13y commented Mar 2, 2022

@hunterhector Still unable to add @tanyuqian as a reviewer. It says I can add atmost 1 reviewer.

@Piyush13y Piyush13y requested a review from hunterhector March 2, 2022 11:33
@hunterhector hunterhector requested review from tanyuqian and removed request for hunterhector March 2, 2022 17:30
@hunterhector
Copy link
Member

hunterhector commented Mar 2, 2022

@hunterhector Still unable to add @tanyuqian as a reviewer. It says I can add almost 1 reviewer.

Add bowen in the future. I will get notified for the PRs anyway

@tanyuqian
Copy link
Member

  1. I guess we want to lint the project with mypy, pylint and black to make it more standard before we merge this.
  2. We will also need to set up test cases (and write test cases), like here.

Pytest and documentation has been updated.
@Piyush13y Could you write unit tests for the components you developed? (mimic_reader and this negation analyzer)

@Piyush13y Piyush13y changed the title Added NegationContextAnalyzer processor Added NegationContextAnalyzer processor and unit tests Mar 4, 2022
@Piyush13y
Copy link
Collaborator Author

@tanyuqian I had already added test cases for NegationContextAnalyzer, and now I have added a test case for Mimic3 reader as well. However, there seems to be something broken with respect to pytest in the workflow I think, which gives errors in the CI pipeline. I am able to run these tests successfully on my local machine.

@tanyuqian tanyuqian merged commit 32049c7 into master Mar 4, 2022
@tanyuqian
Copy link
Member

@tanyuqian I had already added test cases for NegationContextAnalyzer, and now I have added a test case for Mimic3 reader as well. However, there seems to be something broken with respect to pytest in the workflow I think, which gives errors in the CI pipeline. I am able to run these tests successfully on my local machine.

@Piyush13y Could you double-check it is working from your end? like re-install forte/forte-wrapper/forte-medical and re-run? I got the same error message as the workflow here https://github.com/asyml/forte-medical/runs/5435388860?check_suite_focus=true#step:13:23
(ImportError: cannot import name 'MedicalEntityMention' from 'ftx.medical' (/opt/hostedtoolcache/Python/3.8.12/x64/lib/python3.8/site-packages/ftx/medical/__init__.py))

@Piyush13y
Copy link
Collaborator Author

Piyush13y commented Mar 6, 2022

@tanyuqian There were 2 issues raised in the CI pipeline initially, one was due to a stray test folder which, on deletion, fixed that problem.
The other one related to import statement is because this Forte-wrapper's PR is yet to be merged, which completes the namespace consistency story. I have run these tests with that PR and it works fine. There's some other issue which is failing a test on that PR which has blocked its merge for now. @hunterhector mentioned someone will be working on that but might take some time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Negation Context analysis to pipelines
3 participants