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

feat: add classification module for crop orientation #721

Merged
merged 23 commits into from
Dec 20, 2021
Merged

Conversation

charlesmindee
Copy link
Collaborator

This PR adds a classification module with a orientation_classifier which will be used to classify the orientation of crops (0, 90, 180, 270 degrees ccw).
We also provide a rectify_crops function in utils which will be used to rotate the crops accordingly.

Any feedback is welcome!

@charlesmindee charlesmindee added type: enhancement Improvement module: models Related to doctr.models module: utils Related to doctr.utils framework: pytorch Related to PyTorch backend framework: tensorflow Related to TensorFlow backend topic: text detection Related to the task of text detection labels Dec 16, 2021
@charlesmindee charlesmindee self-assigned this Dec 16, 2021
@charlesmindee charlesmindee added this to the 0.5.0 milestone Dec 16, 2021
@codecov
Copy link

codecov bot commented Dec 16, 2021

Codecov Report

Merging #721 (ec5c5ef) into main (2a18dc4) will decrease coverage by 0.12%.
The diff coverage is 94.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #721      +/-   ##
==========================================
- Coverage   96.13%   96.01%   -0.13%     
==========================================
  Files         125      129       +4     
  Lines        4713     4792      +79     
==========================================
+ Hits         4531     4601      +70     
- Misses        182      191       +9     
Flag Coverage Δ
unittests 96.01% <94.62%> (-0.13%) ⬇️

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

Impacted Files Coverage Δ
...octr/models/classification/magc_resnet/__init__.py 100.00% <ø> (ø)
doctr/models/classification/magc_resnet/pytorch.py 100.00% <ø> (ø)
...tr/models/classification/magc_resnet/tensorflow.py 100.00% <ø> (ø)
doctr/models/classification/mobilenet/__init__.py 100.00% <ø> (ø)
doctr/models/classification/predictor/__init__.py 100.00% <ø> (ø)
doctr/models/classification/resnet/__init__.py 100.00% <ø> (ø)
doctr/models/classification/resnet/pytorch.py 100.00% <ø> (ø)
doctr/models/classification/resnet/tensorflow.py 100.00% <ø> (ø)
doctr/models/classification/vgg/pytorch.py 100.00% <ø> (ø)
doctr/models/classification/vgg/tensorflow.py 100.00% <ø> (ø)
... and 21 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 2a18dc4...ec5c5ef. Read the comment docs.

Copy link
Contributor

@fg-mindee fg-mindee left a comment

Choose a reason for hiding this comment

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

Oh my bad, there is a misunderstanding: either we rename backbone into classification (which would make a lot of sense), or we add your modifications to "backbones", but no need to have both I think!

@charlesmindee
Copy link
Collaborator Author

charlesmindee commented Dec 17, 2021

We can merge both but we still need a predictor + model zoo for classification architectures, and we also need to have a cfg proper to this classification task (because the actual cfg of the mobilenet for instance is for a character classification, it is not the same input size, checkpoints, ...)
What do you suggest ?

Copy link
Contributor

@fg-mindee fg-mindee 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 PR! I suggested a few changes!

doctr/models/classification/mobilenet/pytorch.py Outdated Show resolved Hide resolved
doctr/models/classification/mobilenet/pytorch.py Outdated Show resolved Hide resolved
doctr/models/classification/predictor/pytorch.py Outdated Show resolved Hide resolved
tests/pytorch/test_models_backbones_pt.py Outdated Show resolved Hide resolved
tests/pytorch/test_models_classification_pt.py Outdated Show resolved Hide resolved
tests/tensorflow/test_models_backbones_tf.py Outdated Show resolved Hide resolved
Copy link
Contributor

@fg-mindee fg-mindee left a comment

Choose a reason for hiding this comment

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

Thanks! I added a few comments, nothing major. Would you mind adding a corresponding section in the documentation please?

doctr/models/classification/mobilenet/tensorflow.py Outdated Show resolved Hide resolved
doctr/models/classification/mobilenet/tensorflow.py Outdated Show resolved Hide resolved
Copy link
Contributor

@fg-mindee fg-mindee 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, only missing the documentation 👌

Copy link
Contributor

@fg-mindee fg-mindee 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, thanks!

@charlesmindee charlesmindee merged commit 8818a31 into main Dec 20, 2021
@charlesmindee charlesmindee deleted the classif branch December 20, 2021 16:08
@fg-mindee fg-mindee added type: new feature New feature ext: docs Related to docs folder topic: documentation Improvements or additions to documentation and removed type: enhancement Improvement labels Dec 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ext: docs Related to docs folder framework: pytorch Related to PyTorch backend framework: tensorflow Related to TensorFlow backend module: models Related to doctr.models module: utils Related to doctr.utils topic: documentation Improvements or additions to documentation topic: text detection Related to the task of text detection type: new feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants