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

[Fix] TensorFlow SAR_Resnet31 implementation #925

Merged
merged 5 commits into from
May 31, 2022

Conversation

felixdittrich92
Copy link
Contributor

@felixdittrich92 felixdittrich92 commented May 20, 2022

This PR: (Still in progress :))

  • fix SARDecoder forward step

  • check train

  • check inference

  • cleanup / improve code

Any feedback is welcome 🤗

Issue:
#802

@felixdittrich92 felixdittrich92 self-assigned this May 20, 2022
@felixdittrich92 felixdittrich92 added this to the 0.5.2 milestone May 20, 2022
@felixdittrich92 felixdittrich92 added module: models Related to doctr.models framework: tensorflow Related to TensorFlow backend topic: text recognition Related to the task of text recognition labels May 20, 2022
@felixdittrich92
Copy link
Contributor Author

@frgfm
training: Overall it looks good with same config as used for PT implementation it reaches after 17 epochs an exact match from ~70%
inference: i have some problems to translate the torch.scatter_ functionality could you help with this ? :)

@felixdittrich92 felixdittrich92 changed the title [WIP][Fix] TensorFlow SAR_Resnet31 implementation [Fix] TensorFlow SAR_Resnet31 implementation May 23, 2022
@felixdittrich92 felixdittrich92 marked this pull request as ready for review May 23, 2022 10:21
@codecov
Copy link

codecov bot commented May 23, 2022

Codecov Report

Merging #925 (19b287f) into main (0c8dd60) will increase coverage by 0.04%.
The diff coverage is 97.56%.

@@            Coverage Diff             @@
##             main     #925      +/-   ##
==========================================
+ Coverage   94.68%   94.72%   +0.04%     
==========================================
  Files         134      134              
  Lines        5491     5501      +10     
==========================================
+ Hits         5199     5211      +12     
+ Misses        292      290       -2     
Flag Coverage Δ
unittests 94.72% <97.56%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
doctr/models/classification/resnet/tensorflow.py 100.00% <ø> (ø)
doctr/models/recognition/sar/tensorflow.py 99.25% <97.56%> (+0.87%) ⬆️
doctr/models/recognition/sar/pytorch.py 98.50% <0.00%> (-0.02%) ⬇️
doctr/transforms/functional/base.py 97.10% <0.00%> (+1.44%) ⬆️

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 0c8dd60...19b287f. Read the comment docs.

@frgfm
Copy link
Collaborator

frgfm commented May 25, 2022

@frgfm training: Overall it looks good with same config as used for PT implementation it reaches after 17 epochs an exact match from ~70% inference: i have some problems to translate the torch.scatter_ functionality could you help with this ? :)

Could you check the test perf against FUNSD or CORD ? :)
cf. bench https://mindee.github.io/doctr/using_doctr/using_models.html#id5

Copy link
Collaborator

@frgfm frgfm 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 Felix 🙏
I added a few comments!

doctr/models/recognition/sar/pytorch.py Outdated Show resolved Hide resolved
doctr/models/recognition/sar/pytorch.py Outdated Show resolved Hide resolved
doctr/models/recognition/sar/tensorflow.py Outdated Show resolved Hide resolved
@felixdittrich92
Copy link
Contributor Author

@frgfm training: Overall it looks good with same config as used for PT implementation it reaches after 17 epochs an exact match from ~70% inference: i have some problems to translate the torch.scatter_ functionality could you help with this ? :)

Could you check the test perf against FUNSD or CORD ? :) cf. bench https://mindee.github.io/doctr/using_doctr/using_models.html#id5

@frgfm i think this would not be comparable without a full dataset (like mindee intern) :/ I test the implementations currently on a difficult toy dataset 10k train and 2.5k val (contains extrem blurred, rotated images, ~ 30 different fonts and some corrupted characters). Maybe if i found a bit more time i could train it on MJSynth

@felixdittrich92
Copy link
Contributor Author

@frgfm ok now with kwargs and check 👍 😅

@frgfm
Copy link
Collaborator

frgfm commented May 30, 2022

FYI: this PR introduces the deprecation of resnet31 as pretrained model for TF

@felixdittrich92
Copy link
Contributor Author

felixdittrich92 commented May 30, 2022

FYI: this PR introduces the deprecation of resnet31 as pretrained model for TF

@frgfm
Yes i know, but (correct me if i'm wrong) it is currently only used as backbone for SAR (and master which will change before next release also) so i would say lets include this for next training iteration

trained 1 Epoch on MJSynth Subset : 500k train / 200k val
Validation loss decreased inf --> 0.215485: saving state...
Epoch 1/100 - Validation loss: 0.215485 (Exact: 72.99% | Partial: 77.53%)

FUNSD:
Validation loss: 1.89441 (Exact: 44.94% | Partial: 47.41%)
CORD:
Validation loss: 2.34539 (Exact: 36.30% | Partial: 36.77%)

I would say for a toy run this looks really not bad and we can close the SAR issue with this PR wdyt ? :)

@felixdittrich92 felixdittrich92 added critical High priority type: breaking change Introducing a breaking change labels May 30, 2022
Copy link
Collaborator

@frgfm frgfm left a comment

Choose a reason for hiding this comment

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

Thanks Felix, I added some comments!

doctr/models/recognition/sar/tensorflow.py Outdated Show resolved Hide resolved
doctr/models/recognition/sar/tensorflow.py Outdated Show resolved Hide resolved
doctr/models/recognition/sar/tensorflow.py Outdated Show resolved Hide resolved
doctr/models/recognition/sar/tensorflow.py Show resolved Hide resolved
doctr/models/recognition/sar/tensorflow.py Outdated Show resolved Hide resolved
doctr/models/recognition/sar/tensorflow.py Show resolved Hide resolved
doctr/models/recognition/sar/tensorflow.py Outdated Show resolved Hide resolved
doctr/models/recognition/sar/tensorflow.py Show resolved Hide resolved
@frgfm
Copy link
Collaborator

frgfm commented May 30, 2022

FYI: this PR introduces the deprecation of resnet31 as pretrained model for TF

@frgfm Yes i know, but (correct me if i'm wrong) it is currently only used as backbone for SAR (and master which will change before next release also) so i would say lets include this for next training iteration

trained 1 Epoch on MJSynth Subset : 500k train / 200k val Validation loss decreased inf --> 0.215485: saving state... Epoch 1/100 - Validation loss: 0.215485 (Exact: 72.99% | Partial: 77.53%)

FUNSD: Validation loss: 1.89441 (Exact: 44.94% | Partial: 47.41%) CORD: Validation loss: 2.34539 (Exact: 36.30% | Partial: 36.77%)

I would say for a toy run this looks really not bad and we can close the SAR issue with this PR wdyt ? :)

I agree, that's quite good 👍

@frgfm frgfm added the type: bug Something isn't working label May 30, 2022
Copy link
Collaborator

@frgfm frgfm left a comment

Choose a reason for hiding this comment

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

Just a few typos left!

doctr/models/classification/resnet/tensorflow.py Outdated Show resolved Hide resolved
doctr/models/recognition/sar/tensorflow.py Outdated Show resolved Hide resolved
doctr/models/recognition/sar/tensorflow.py Outdated Show resolved Hide resolved
@felixdittrich92
Copy link
Contributor Author

@frgfm
Are we good ? :)

Copy link
Collaborator

@frgfm frgfm left a comment

Choose a reason for hiding this comment

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

Thanks Felix 🙏

@frgfm frgfm merged commit 75531c5 into mindee:main May 31, 2022
@felixdittrich92 felixdittrich92 deleted the fix-sar-tf branch May 31, 2022 10:38
@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
critical High priority framework: tensorflow Related to TensorFlow backend module: models Related to doctr.models topic: text recognition Related to the task of text recognition type: breaking change Introducing a breaking change type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants