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

Add an example to incorporate remote clinical pipeline and stave #43

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Add an example to incorporate remote clinical pipeline and stave #43

wants to merge 22 commits into from

Conversation

Leolty
Copy link
Collaborator

@Leolty Leolty commented Jul 9, 2022

This PR fixes #42.

Description of changes

This PR is totally based on what has been done by Piyush, here is the reference: https://github.com/Piyush13y/forte/tree/745_clinical_pipeline_stave

The changes include:

  1. The JSON files are serialized in the wrong way, and I fix this.
  2. I modify some processors in the clinical_processing_pipeline.py, and add an argument to support PlainTextReader(). In case users may not have the mimic iii datasets, they can just use the sample data in the sample_data/notes.txt for the function test.
  3. I add more instructions in README.md, according to the errors or obstacles I encountered when I set up the connection between the pipeline and stave.
  4. I remove some files that were not used.

Bugs encountered

Initially, the version of Elasticsearch I downloaded is 8.2.2, and I failed the setup of this according to the tutorials. ( Many unsolvable bugs). When I change the version to 7.12.2 (given in README.md), everything works perfectly fine.

@codecov
Copy link

codecov bot commented Jul 9, 2022 •
edited
Loading

Codecov Report

Merging #43 (cedb045) into master (9ecd4a0) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #43      +/-   ##
==========================================
- Coverage   84.17%   84.14%   -0.04%     
==========================================
  Files           9        8       -1     
  Lines         512      511       -1     
==========================================
- Hits          431      430       -1     
  Misses         81       81
Impacted Files Coverage Δ
...te_medical/processors/icd_coding_processor_test.py
...s/forte_medical/readers/mimic3_note_reader_test.py
...dical/processors/negation_context_analysis_test.py
...tex/health/processors/icd_coding_processor_test.py 100.00% <0.00%> (ø)
...ealth/processors/negation_context_analysis_test.py 100.00% <0.00%> (ø)
...s/fortex/health/readers/mimic3_note_reader_test.py 96.29% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us.

@Piyush13y Piyush13y requested a review from hunterhector July 11, 2022 07:23
hunterhector
hunterhector reviewed Jul 11, 2022
View reviewed changes
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.

A few comments, but overall, if this example is runnable it is fine with me. We can improve in the future.

examples/clinical_pipeline/utterance_searcher.py Outdated Show resolved Hide resolved
examples/clinical_pipeline/mimic3_note_reader.py Outdated Show resolved Hide resolved
examples/clinical_pipeline/default_onto_project.json Outdated Show resolved Hide resolved
examples/clinical_pipeline/chat_project.json Outdated Show resolved Hide resolved
@Leolty
Copy link
Collaborator Author

Leolty commented Jul 17, 2022

Could we leave these two issues metioned in the conversations in the future? I am not familiar with the ontology generation yet so it may take much time.

@hunterhector
Copy link
Member

Could we leave these two issues metioned in the conversations in the future? I am not familiar with the ontology generation yet so it may take much time.

Actually, it might be important to understand the following items since they are the key for understanding our visualization:

  1. Be able to update, and generate ontology
  2. Understand how ontologies are related to Stave settings (examples/clinical_pipeline/chat_project.json)

@Leolty
Copy link
Collaborator Author

Leolty commented Jul 19, 2022

Could we leave these two issues metioned in the conversations in the future? I am not familiar with the ontology generation yet so it may take much time.

Actually, it might be important to understand the following items since they are the key for understanding our visualization:

  1. Be able to update, and generate ontology
  2. Understand how ontologies are related to Stave settings (examples/clinical_pipeline/chat_project.json)

Got it. Actually, I understand how the ontologies related to stave. I didn't understand what you meant at first, I thought it was about writing python code to automatically generate ontologies. So I think what you want me to do is to omit most of the ontologies that were already written, write only the extra ones we added here, and then use the generate ontology function to merge them?

@Leolty Leolty closed this Jul 19, 2022
@Leolty Leolty reopened this Jul 19, 2022
@hunterhector
Copy link
Member

Could we leave these two issues metioned in the conversations in the future? I am not familiar with the ontology generation yet so it may take much time.

Actually, it might be important to understand the following items since they are the key for understanding our visualization:

  1. Be able to update, and generate ontology
  2. Understand how ontologies are related to Stave settings (examples/clinical_pipeline/chat_project.json)

Got it. Actually, I understand how the ontologies related to stave. I didn't understand what you meant at first, I thought it was about writing python code to automatically generate ontologies. So I think what you want me to do is to omit most of the ontologies that were already written, write only the extra ones we added here, and then use the generate ontology function to merge them?

Yeah I think you got the idea, basically we don’t need to repeat the ontology spec here again, and be able to comtrol the ontology should help you demo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Piyush13y Piyush13y Piyush13y left review comments

@hunterhector hunterhector hunterhector left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add an example for clinical pipeline and stave
3 participants
@Leolty @hunterhector @Piyush13y