Skip to content

[Fix] Tensorflow MASTER implementation #949

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

Merged
merged 11 commits into from
Jul 1, 2022

Conversation

felixdittrich92
Copy link
Contributor

@felixdittrich92 felixdittrich92 commented Jun 13, 2022

This PR:

  • refactored tensorflow transformer implementation
  • fix MASTER Decoder

Any feedback is welcome 🤗
@charlesmindee @frgfm feel free to take a first look some improvements are very welcome :D

Todo:

@felixdittrich92 felixdittrich92 added this to the 0.5.2 milestone Jun 13, 2022
@felixdittrich92 felixdittrich92 added type: bug Something isn't working module: models Related to doctr.models framework: tensorflow Related to TensorFlow backend topic: text recognition Related to the task of text recognition labels Jun 13, 2022
@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #949 (760ceb3) into main (82d6e7e) will increase coverage by 0.26%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #949      +/-   ##
==========================================
+ Coverage   94.89%   95.16%   +0.26%     
==========================================
  Files         134      134              
  Lines        5542     5520      -22     
==========================================
- Hits         5259     5253       -6     
+ Misses        283      267      -16     
Flag Coverage Δ
unittests 95.16% <100.00%> (+0.26%) ⬆️

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

Impacted Files Coverage Δ
...tr/models/classification/magc_resnet/tensorflow.py 100.00% <ø> (ø)
doctr/models/recognition/master/pytorch.py 95.32% <ø> (ø)
doctr/models/recognition/transformer/pytorch.py 100.00% <ø> (ø)
doctr/models/recognition/master/tensorflow.py 100.00% <100.00%> (+2.80%) ⬆️
doctr/models/recognition/transformer/tensorflow.py 100.00% <100.00%> (+11.88%) ⬆️
doctr/transforms/functional/base.py 95.65% <0.00%> (-1.45%) ⬇️
doctr/transforms/modules/base.py 94.59% <0.00%> (ø)
doctr/transforms/modules/tensorflow.py 83.51% <0.00%> (+1.09%) ⬆️

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 82d6e7e...760ceb3. Read the comment docs.

@felixdittrich92
Copy link
Contributor Author

@frgfm If you should get to it before me, please take a look at the decoding and code improvements in advance would also be quite good 👍

@felixdittrich92 felixdittrich92 requested a review from frgfm June 13, 2022 19:32
@felixdittrich92
Copy link
Contributor Author

felixdittrich92 commented Jun 15, 2022

MJSynth 500K train 100K val
Train set loaded in 311.6s (509998 samples in 3984 batches)
Validation loss decreased inf --> 0.000531279: saving state...
Epoch 1/1 - Validation loss: 0.000531279 (Exact: 99.86% | Partial: 99.86%)

FUNSD:
Validation loss: 1.01889 (Exact: 71.46% | Partial: 71.46%)
CORD:
Validation loss: 0.8728 (Exact: 63.35% | Partial: 63.35%)

But decoding while inference is still broken :
single (recognition_predictor):
[('8555555ySIXIyddy', 0.30775195360183716)]

complete pipe:

Block(
        (lines): [Line(
          (words): [
            Word(value='85555555SIXI5Xd', confidence=0.2),
            Word(value='8555555ySIXIyddy', confidence=0.3),
            Word(value='85555555SIXI5Xyyy', confidence=0.19),
            Word(value='85555555SIXI5Xyyy', confidence=0.19),
            Word(value='85555555SIXI5Xyyy', confidence=0.19),
          ]
        )]

@frgfm @charlesmindee i don't see what i missed, so i think a second pair of eyes would be quite good 👀

I would recommend that one of you two takes this stand and validates it again, because there is a clear difference to the toy benchmark on the pyTorch side implementation which works fine 😅

@felixdittrich92 felixdittrich92 added the help wanted Extra attention is needed label Jun 15, 2022
@felixdittrich92
Copy link
Contributor Author

@frgfm this PR would be very happy to get your help 🤣

@felixdittrich92 felixdittrich92 self-assigned this Jun 27, 2022
@felixdittrich92 felixdittrich92 removed the help wanted Extra attention is needed label Jun 30, 2022
@felixdittrich92
Copy link
Contributor Author

felixdittrich92 commented Jun 30, 2022

Again 500K MJSynth toy run:
Validation loss decreased inf --> 0.237682: saving state...
Epoch 1/10 - Validation loss: 0.237682 (Exact: 71.12% | Partial: 74.52%)
FUNSD: Validation loss: 2.02109 (Exact: 41.82% | Partial: 44.07%)
CORD: Validation loss: 2.76744 (Exact: 28.25% | Partial: 29.00%)

Inference:

Line(
(words): [
Word(value='Die', confidence=0.57),
Word(value='jeweils', confidence=0.61),
Word(value='gewahlte', confidence=0.49),
Word(value='Ausfuhrung', confidence=0.99),
]
),

)
pe = pe.numpy()
pe[:, 0::2] = tf.math.sin(position * div_term)
pe[:, 1::2] = tf.math.cos(position * div_term)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@frgfm Any idea how to do this in TF and remove the numpy cast? 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @felixdittrich92, I know this but I think it is still a bit experimental

@felixdittrich92 felixdittrich92 marked this pull request as ready for review June 30, 2022 11:11
@felixdittrich92 felixdittrich92 changed the title [WIP][Fix] Tensorflow MASTER implementation [Fix] Tensorflow MASTER implementation Jun 30, 2022
@felixdittrich92
Copy link
Contributor Author

Note:
mypy will be fixed with #966
train-char-classification ref. #949 @frgfm (it isn't fixed with the upgrade 😅) i think best would be as mentioned to replace mobilenet with keras.applications implementation soon

charlesmindee
charlesmindee previously approved these changes Jul 1, 2022
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, LGTM!

)
pe = pe.numpy()
pe[:, 0::2] = tf.math.sin(position * div_term)
pe[:, 1::2] = tf.math.cos(position * div_term)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @felixdittrich92, I know this but I think it is still a bit experimental

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! LGTM

@felixdittrich92 felixdittrich92 merged commit 9530f81 into mindee:main Jul 1, 2022
@felixdittrich92 felixdittrich92 deleted the fix-master-tf branch July 1, 2022 18:07
@felixdittrich92 felixdittrich92 modified the milestones: 0.5.2, 0.6.0 Sep 26, 2022
@felixdittrich92 felixdittrich92 mentioned this pull request Sep 26, 2022
85 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework: tensorflow Related to TensorFlow backend module: models Related to doctr.models topic: text recognition Related to the task of text recognition type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants