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

added image processing class to preprocess the image before running OCR #1166

Merged
merged 22 commits into from
Oct 4, 2023
Merged

added image processing class to preprocess the image before running OCR #1166

merged 22 commits into from
Oct 4, 2023

Conversation

ayabel
Copy link
Collaborator

@ayabel ayabel commented Sep 5, 2023

Change Description

Added an option to run an image preprocessor in the image analyzer engine

Issue reference

This PR fixes issue #XX

Checklist

  • [x ] I have reviewed the contribution guidelines
  • I have signed the CLA (if required)
  • [x ] My code includes unit tests
  • [x ] All unit tests and lint checks pass locally
  • [x ] My PR contains documentation updates / additions if required

niwilso
niwilso requested changes Sep 5, 2023
View reviewed changes
Copy link
Collaborator

@niwilso niwilso left a comment

Choose a reason for hiding this comment

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

Overall, this looks great! Just left some comments to be addressed before approval.

@ayabel ayabel requested a review from a team as a code owner September 6, 2023 07:17
@omri374
Copy link
Contributor

omri374 commented Sep 6, 2023

@ayabel looks like the e2e tests are failing. Have you looked into this? Is it because the new logic changes the output?

omri374
omri374 previously approved these changes Sep 6, 2023
View reviewed changes
Copy link
Contributor

@omri374 omri374 left a comment

Choose a reason for hiding this comment

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

Looks great! Added a few minor comments

niwilso
niwilso previously approved these changes Sep 6, 2023
View reviewed changes
Copy link
Collaborator

@niwilso niwilso left a comment

Choose a reason for hiding this comment

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

Looks good to me now! Will re-approve as needed if code changes are made to address the failing e2e tests.

@niwilso niwilso mentioned this pull request Sep 7, 2023
Closed
5 tasks
@ayabel ayabel dismissed stale reviews from niwilso and omri374 via e24ce0b September 11, 2023 20:36
niwilso
niwilso previously approved these changes Sep 11, 2023
View reviewed changes
@ayabel ayabel requested a review from omri374 September 11, 2023 21:47
niwilso
niwilso previously approved these changes Sep 28, 2023
View reviewed changes
Copy link
Collaborator

@niwilso niwilso left a comment

Choose a reason for hiding this comment

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

Minor comments but otherwise good to go

niwilso
niwilso previously approved these changes Sep 28, 2023
View reviewed changes
omri374
omri374 previously approved these changes Oct 4, 2023
View reviewed changes
@omri374 omri374 dismissed stale reviews from niwilso and themself via 023228a October 4, 2023 11:19
omri374
omri374 previously approved these changes Oct 4, 2023
View reviewed changes
@omri374
Update install_nlp_models.py
0c54a99 
removed unneeded spacy import
@omri374 omri374 merged commit 6cf5f18 into main Oct 4, 2023
@omri374 omri374 deleted the ayabellicha/dicom/text-detection-improvements branch October 4, 2023 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omri374 omri374 omri374 approved these changes

@niwilso niwilso niwilso left review comments

@SharonHart SharonHart Awaiting requested review from SharonHart

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

Successfully merging this pull request may close these issues.
3 participants
@ayabel @omri374 @niwilso