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: ✨ allow beam width > 1 in the CRNN postprocessor #630

Merged
merged 3 commits into from
Apr 11, 2022

Conversation

khalidMindee
Copy link
Contributor

This PR aims to allow beam search decoding in CRNN postprocessing , instead of only greedy decoding ( corresponding to a beam search with beam width equal to 1 )

@codecov
Copy link

codecov bot commented Nov 16, 2021

Codecov Report

Merging #630 (8d4a8c1) into main (b85d8c8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 8d4a8c1 differs from pull request most recent head 5aeb02d. Consider uploading reports for the commit 5aeb02d to get more accurate results

@@           Coverage Diff           @@
##             main     #630   +/-   ##
=======================================
  Coverage   94.92%   94.93%           
=======================================
  Files         133      133           
  Lines        5281     5288    +7     
=======================================
+ Hits         5013     5020    +7     
  Misses        268      268           
Flag Coverage Δ
unittests 94.93% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
doctr/models/recognition/crnn/tensorflow.py 100.00% <100.00%> (ø)
doctr/transforms/modules/base.py 94.59% <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 b85d8c8...5aeb02d. Read the comment docs.

@fg-mindee fg-mindee self-assigned this Nov 17, 2021
@fg-mindee fg-mindee added topic: text recognition Related to the task of text recognition type: enhancement Improvement labels Nov 17, 2021
@fg-mindee fg-mindee self-requested a review November 17, 2021 11:59
@khalidMindee
Copy link
Contributor Author

This PR solves #602 for exporting CRNN recognition models as saved models .

Copy link
Collaborator

@charlesmindee charlesmindee 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 this feature, but we may want to keep the same input/output format to avoid breaking the predictors

doctr/models/recognition/crnn/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.

As Charles mentioned, there are some parts of this PR that could be integrated and others that introduce breaking changes, that cannot be handled right now :/

I think we should split this into 2 PRs:

  • one to fix SavedModel compatibility
  • one to add topk

doctr/models/recognition/crnn/tensorflow.py Show resolved Hide resolved
doctr/models/recognition/crnn/tensorflow.py Outdated Show resolved Hide resolved
doctr/models/recognition/crnn/tensorflow.py Outdated Show resolved Hide resolved
doctr/models/recognition/crnn/tensorflow.py Show resolved Hide resolved
doctr/models/recognition/crnn/tensorflow.py Outdated Show resolved Hide resolved
doctr/models/recognition/crnn/tensorflow.py Outdated Show resolved Hide resolved
doctr/models/recognition/crnn/tensorflow.py Outdated Show resolved Hide resolved
@fg-mindee fg-mindee added the type: breaking change Introducing a breaking change label Dec 23, 2021
@fg-mindee fg-mindee added the wontfix This will not be worked on label Feb 18, 2022
@fharper
Copy link
Contributor

fharper commented Apr 8, 2022

@khalidMindee is this PR still good? If yes, can you review FG's & @charlesmindee comments, modify in consequences so we can potentially merge it please?

@khalidMindee
Copy link
Contributor Author

This PR has been updated accordingly to keep only allowing beam width , postponning the fix for exporting the saved model as it introduces a breaking change due to signature change

… add unit test for inference using crnn with beam_search
Copy link
Collaborator

@charlesmindee charlesmindee left a comment

Choose a reason for hiding this comment

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

Thanks!

tests/tensorflow/test_models_recognition_tf.py Outdated Show resolved Hide resolved
@charlesmindee charlesmindee merged commit 8c39cec into main Apr 11, 2022
@charlesmindee charlesmindee deleted the crnn-beam-search branch April 11, 2022 14:46
@github-actions
Copy link

Hey @charlesmindee 👋
You merged this PR, but it is not correctly labeled. The list of valid labels is available at https://github.com/mindee/doctr/blob/main/.github/verify_pr_labels.py

@charlesmindee charlesmindee removed the wontfix This will not be worked on label Apr 11, 2022
@frgfm
Copy link
Collaborator

frgfm commented Apr 27, 2022

Missing two labels "module: models" and "ext: test" I think @charlesmindee :)

@frgfm frgfm added module: models Related to doctr.models ext: tests Related to tests folder labels May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ext: tests Related to tests folder module: models Related to doctr.models topic: text recognition Related to the task of text recognition type: breaking change Introducing a breaking change type: enhancement Improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants