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

Add User friendly API #366

Merged
merged 14 commits into from Jul 15, 2021
Merged

Add User friendly API #366

merged 14 commits into from Jul 15, 2021

Conversation

samayala22
Copy link
Contributor

@samayala22 samayala22 commented Jul 13, 2021

Adds a file that can be called from the command line or by calling the module from an external script and performs end to end ocr much more easily than using the demo file. In my opinion, this would make mmocr much more accessible to beginners and people who are interested in quickly trying out the implemented algorithms.

Example:

from ocr import MMOCR

ocr = MMOCR(textdet='MaskRCNN_ICDAR17', textrecog='SEG')
results = ocr.readtext('.\\demo\\demo_text_recog.jpg', out_img='.\\demo\\out.jpg', details=False)

or

python ocr.py demo\demo_text_recog.jpg --textdet PS_ICDAR15 --textrecog CRNN --imshow --details

All the arguments are pretty much self-explanatory.
The detection and recognition models are listed at the beginning of the file.

The API has been designed to be easily modified and adding arguments is as easy as adding the new arg in the parse_args method, adding the new arg in the arguments of the readtext method inside the MMOCR class. You can now access the value of the argument with args.<new_arg> inside the readtext method.

I have already mentioned the performance issues in #335 (but nobody addressed it) and I'm pretty sure this has something to do with the implementation of the models and not the API.

I'm not a programmer and I didn't fully understand all the contributing guidelines steps, my apologies in advance if this PR is not formatted correctly.

@CLAassistant
Copy link

CLAassistant commented Jul 13, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@gaotongxiao gaotongxiao left a comment

Choose a reason for hiding this comment

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

Thanks for the nice work! That would be really helpful for users. Just a few comments:

  1. Is it necessary to rename the config files? If not, sticking to original names is safer as it will not break the code;
  2. The script could be moved to tools/
  3. It would be great if you also add a short intro section to getting_started.md (https://mmocr.readthedocs.io/en/latest/getting_started.html#useful-tools). If that's too much for you, we can also do it later.
  4. There might be some minor problems that cannot pass our linting test (e.g. the file doesn't end with a new line) but can be fixed by pre-commit hook. See (https://github.com/open-mmlab/mmocr/blob/main/.github/CONTRIBUTING.md#python and https://github.com/open-mmlab/mmocr/blob/main/.github/CONTRIBUTING.md#step-3-commit-your-changes for details). In short, try
pre-commit install
pre-commit run --all-files

@samayala22
Copy link
Contributor Author

Thanks for the quick response !

  1. Not it's not necessary, I did it to improve consistency between the repo naming structure and the one in the openmmlab server: https://download.openmmlab.com/mmocr/textrecog/robustscanner/robustscanner_r31_academic-5f05874f.pth. But I understand the concerns and will stick with the original name of the config directory.
  2. Alright, I tried it earlier but I kept getting ModuleNotFoundError: No module named 'mmocr' and after trying a lot of possible solutions unsuccessfully I just placed it in the root directory. I will try again tomorrow.
  3. Got it, will do it tomorrow.
  4. Noted, I'll do it too.

@codecov
Copy link

codecov bot commented Jul 14, 2021

Codecov Report

Merging #366 (418b637) into main (782e966) will decrease coverage by 1.41%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #366      +/-   ##
==========================================
- Coverage   84.81%   83.40%   -1.42%     
==========================================
  Files         136      137       +1     
  Lines        9100     9254     +154     
  Branches     1281     1318      +37     
==========================================
  Hits         7718     7718              
- Misses       1077     1231     +154     
  Partials      305      305              
Flag Coverage Δ
unittests 83.40% <0.00%> (-1.42%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mmocr/utils/ocr.py 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 782e966...418b637. Read the comment docs.

Copy link
Collaborator

@gaotongxiao gaotongxiao left a comment

Choose a reason for hiding this comment

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

I found it work well without the extra path insertion lines. Has your mmocr been correctly installed, or you are just running it as a standalone repo?

tools/ocr.py Outdated Show resolved Hide resolved
tools/ocr.py Outdated Show resolved Hide resolved
@samayala22
Copy link
Contributor Author

I found it work well without the extra path insertion lines. Has your mmocr been correctly installed, or you are just running it as a standalone repo?

mmocr does not install on windows so I just cloned the repo in my documents directory. I guess it should work fine on linux then.

- Moved file to mmocr/utils
- Added support for custom config files
@samayala22
Copy link
Contributor Author

samayala22 commented Jul 14, 2021

I moved the file to mmocr/utils, modified the mmocr/utils/__init__.py and added arguments for custom detection and recognition configs. If there's anything else you think I should add as an argument let me know. I was thinking of adding support for

  • Choosing folder of images to run inference on
  • Choose between det + recog, det only or recog only
  • Option to save results as .txt file

Is this something you think is worth adding ?

@gaotongxiao
Copy link
Collaborator

The first and second options sound really great! For the third one, I think supporting .json file is enough cuz we don't really have to support every possible format. Yet it would be a nice bonus.

In fact, the script looks good enough to be merged. But your codebase is a bit outdated, so it would be great to locally resolve the conflict with the latest main branch first.

- Adds support for separate detection and recognition
- A bit of refactoring
- Moved file to mmocr/utils
- Added support for custom config files
- Adds support for separate detection and recognition
- A bit of refactoring
Comment on lines 12 to 13
from .ocr import MMOCR
from .string_util import StringStrip
Copy link
Collaborator

Choose a reason for hiding this comment

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

Try switching their order to pass the test (could avoid circular import)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to change it but when I try to commit the change isort automatically changes it

flake8...................................................................Passed
seed isort known_third_party.............................................Passed
isort....................................................................Failed
- hook id: isort
- files were modified by this hook

Fixing mmocr\utils\__init__.py

Copy link
Collaborator

@gaotongxiao gaotongxiao left a comment

Choose a reason for hiding this comment

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

LGTM though circular import still doesn't get fixed. We may change the directory structure in the future.

@gaotongxiao gaotongxiao merged commit 87bd295 into open-mmlab:main Jul 15, 2021
@gaotongxiao
Copy link
Collaborator

BTW would you plan to write something about your script in the docs?

@samayala22
Copy link
Contributor Author

samayala22 commented Jul 15, 2021

Yep, I will write something. Where do you think is best ?

My script is basically an improved version of the demo files because it combines them in a single file and adds some functionalities. So, perhaps I should change the demo tab in the docs ? And maybe delete the superseded demo files ?

@gaotongxiao
Copy link
Collaborator

Great! You can just change the content in this file https://github.com/open-mmlab/mmocr/blob/main/demo/docs/ocr_demo.md. Also, ocr_image_demo.py is not needed and can be deleted now

gaotongxiao pushed a commit to gaotongxiao/mmocr that referenced this pull request Jul 15, 2022
gaotongxiao pushed a commit to gaotongxiao/mmocr that referenced this pull request Jul 15, 2022
@OpenMMLab-Coodinator
Copy link

Hi @samayala22 !First of all, we want to express our gratitude for your significant PR in this project. Your contribution is highly appreciated, and we are grateful for your efforts in helping improve this open-source project during your personal time. We believe that many developers will benefit from your PR.

We would also like to invite you to join our Special Interest Group (SIG) private channel on Discord, where you can share your experiences, ideas, and build connections with like-minded peers. To join the SIG channel, simply message moderator— OpenMMLab on Discord or briefly share your open-source contributions in the #introductions channel and we will assist you. Look forward to seeing you there! Join us :https://discord.gg/UjgXkPWNqA

If you have WeChat account,welcome to join our community on WeChat. You can add our assistant :openmmlabwx. Please add "mmsig + Github ID" as a remark when adding friends:)
Thank you again for your contribution❤

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.

None yet

4 participants