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 TPS #117

Merged
merged 8 commits into from
Apr 28, 2021
Merged

Add TPS #117

merged 8 commits into from
Apr 28, 2021

Conversation

2793145003
Copy link
Contributor

  • Add TPS as a preprocessor
    (But I'm not sure if I write the source correctly...)

@CLAassistant
Copy link

CLAassistant commented Apr 22, 2021

CLA assistant check
All committers have signed the CLA.

@innerlee innerlee requested a review from cuhk-hbsun April 22, 2021 12:04
output:
batch_I_r: rectified image [batch_size x I_channel_num x I_r_height x I_r_width]
"""
super(TPSPreprocessor, self).__init__()
Copy link
Contributor

Choose a reason for hiding this comment

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

super().__init__()

class TPSPreprocessor(BasePreprocessor):
""" Rectification Network of RARE, namely TPS based STN """

def __init__(self, F, I_size, I_r_size, I_channel_num=1):
Copy link
Contributor

Choose a reason for hiding this comment

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

F

Typically we do not use a single letter as variable name. Need to make their names more descriptive.

def __init__(self, F, I_size, I_r_size, I_channel_num=1):
""" Based on RARE TPS
input:
batch_I: Batch Input Image [batch_size x I_channel_num x I_height x I_width]
Copy link
Contributor

Choose a reason for hiding this comment

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

The docstring format should be adapted to our style (similar to pytorch's) :)

@codecov
Copy link

codecov bot commented Apr 22, 2021

Codecov Report

Merging #117 (f8322db) into main (43dcb32) will increase coverage by 0.37%.
The diff coverage is 84.05%.

❗ Current head f8322db differs from pull request most recent head ca79c92. Consider uploading reports for the commit ca79c92 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #117      +/-   ##
==========================================
+ Coverage   76.58%   76.96%   +0.37%     
==========================================
  Files         105      107       +2     
  Lines        6384     6489     +105     
  Branches      939      943       +4     
==========================================
+ Hits         4889     4994     +105     
  Misses       1280     1280              
  Partials      215      215              
Flag Coverage Δ
unittests 76.96% <84.05%> (+0.37%) ⬆️

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

Impacted Files Coverage Δ
mmocr/datasets/pipelines/ocr_transforms.py 67.21% <ø> (ø)
mmocr/models/textdet/losses/db_loss.py 15.78% <ø> (-1.10%) ⬇️
mmocr/models/textdet/postprocess/wrapper.py 45.37% <0.00%> (ø)
mmocr/utils/lmdb_util.py 74.28% <0.00%> (ø)
mmocr/datasets/pipelines/dbnet_transforms.py 13.29% <14.28%> (ø)
mmocr/core/visualize.py 91.10% <66.66%> (ø)
mmocr/apis/inference.py 89.47% <100.00%> (ø)
mmocr/models/textdet/necks/fpn_cat.py 100.00% <100.00%> (ø)
mmocr/models/textrecog/backbones/resnet31_ocr.py 100.00% <100.00%> (ø)
mmocr/models/textrecog/necks/fpn_ocr.py 100.00% <100.00%> (ø)
... and 4 more

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 43dcb32...ca79c92. Read the comment docs.

@cuhk-hbsun
Copy link
Collaborator

ci_lint is failed, please run pre-commit run --all-files before git commit

@innerlee
Copy link
Contributor

The linting fails. Please run

pre-commit install
pre-commt run --all-files

at the root of the repo

@2793145003
Copy link
Contributor Author

Thanks for the relpy.
Let me know if there's any other problem.

@innerlee
Copy link
Contributor

The next step is to add some unittests. The goal is to cover the new code as fully as possible. You can check the coverage status from #117 (comment) and links therein.

def __init__(self, num_fiducial, I_size, I_r_size, I_channel_num=1):
""" Based on RARE TPS
Args:
num_fiducial: number of fiducial points of TPS-STN
Copy link
Contributor

Choose a reason for hiding this comment

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

Annotate the types of args

""" Based on RARE TPS
Args:
num_fiducial: number of fiducial points of TPS-STN
I_size : (height, width) of the input image I
Copy link
Contributor

Choose a reason for hiding this comment

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

I_size -> img_size

Args:
num_fiducial: number of fiducial points of TPS-STN
I_size : (height, width) of the input image I
I_r_size : (height, width) of the rectified image I_r
Copy link
Contributor

Choose a reason for hiding this comment

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

I_r_size -> rectified_img_size

num_fiducial: number of fiducial points of TPS-STN
I_size : (height, width) of the input image I
I_r_size : (height, width) of the rectified image I_r
I_channel_num : the number of channels of the input image I
Copy link
Contributor

@innerlee innerlee Apr 22, 2021

Choose a reason for hiding this comment

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

I_channel_num -> num_img_channel

super().__init__()
self.num_fiducial = num_fiducial
self.I_size = I_size
self.I_r_size = I_r_size # = (I_r_height, I_r_width)
Copy link
Contributor

Choose a reason for hiding this comment

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

rm the comment since they are in docstring

@2793145003
Copy link
Contributor Author

well I tried. let me know if I write it wrong...

(img_width x img_height)"""

def __init__(self, num_fiducial, num_img_channel):
super(LocalizationNetwork, self).__init__()
Copy link
Contributor

Choose a reason for hiding this comment

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

super().init()


def __init__(self, num_fiducial, rectified_img_size):
"""Generate P_hat and inv_delta_C for later."""
super(GridGenerator, self).__init__()
Copy link
Contributor

Choose a reason for hiding this comment

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

super().init()

@innerlee
Copy link
Contributor

Thanks @2793145003 , we have taken a pass on coding styles. I will take another pass on the functionality.

Besides, since this is a model, a config containg this module is needed, so that users know how to use it. Some documentation on what it is, and how to use it will be great. We can offer help regarding these two

@2793145003
Copy link
Contributor Author

I'm wondering wether should I put the config file in crnn/ or in a new folder, since TPS is just a preprocessor and can be used in many models.
And I used TPS in my own dataset, so I don't have results that can be shown...

@innerlee
Copy link
Contributor

A new folder is okay. If the config is modified from crnn/crnn_academic_dataset.py, then the new config could be tps/crnn_tps_academic_dataset.py.
The configs/textrecog/crnn/README.md could also be created to give the paper information.

@2793145003
Copy link
Contributor Author

I added the paper information, but it seems too crude, any advice?

@innerlee
Copy link
Contributor

The paper info is okay. We will also have a section of results on academic datasets and provide training logs and checkpoints. Do you want to train one model on academic dataset, and report the results here? If not, don't worry, we will verify this later.

@2793145003
Copy link
Contributor Author

well, my gpu is being used for trying to get better result on my own dataset, maybe I can train on academic dataset after I finish it but I'm not sure when I can do it...

@cuhk-hbsun cuhk-hbsun merged commit 7bf88d0 into open-mmlab:main Apr 28, 2021
gaotongxiao pushed a commit to gaotongxiao/mmocr that referenced this pull request Jul 15, 2022
* Add TPS

* Update tps_preprocessor.py

* Add licence, change variable name and format

* renamed some parameters and add tests of ocr preprocessor

* renamed params

* Update tps_preprocessor.py

* add config file and readme for TPS
gaotongxiao pushed a commit to gaotongxiao/mmocr that referenced this pull request Jul 15, 2022
* Add TPS

* Update tps_preprocessor.py

* Add licence, change variable name and format

* renamed some parameters and add tests of ocr preprocessor

* renamed params

* Update tps_preprocessor.py

* add config file and readme for TPS
@OpenMMLab-Coodinator
Copy link

Hi @2793145003 !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.

5 participants