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 dropout in MASTER #349

Merged
merged 4 commits into from
Jul 6, 2021
Merged

feat: add dropout in MASTER #349

merged 4 commits into from
Jul 6, 2021

Conversation

charlesmindee
Copy link
Collaborator

This PR adds dropout in both tf & pytorch MASTER models
Any feedback is welcome!

@charlesmindee charlesmindee self-assigned this Jul 5, 2021
@charlesmindee charlesmindee added type: enhancement Improvement framework: pytorch Related to PyTorch backend framework: tensorflow Related to TensorFlow backend module: models Related to doctr.models labels Jul 5, 2021
@charlesmindee charlesmindee added this to the 0.3.1 milestone Jul 5, 2021
@codecov
Copy link

codecov bot commented Jul 5, 2021

Codecov Report

Merging #349 (b5e3b3e) into main (b2ded17) will increase coverage by 0.65%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #349      +/-   ##
==========================================
+ Coverage   95.40%   96.06%   +0.65%     
==========================================
  Files          83       83              
  Lines        3396     3404       +8     
==========================================
+ Hits         3240     3270      +30     
+ Misses        156      134      -22     
Flag Coverage Δ
unittests 96.06% <100.00%> (+0.65%) ⬆️

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

Impacted Files Coverage Δ
doctr/models/recognition/master/pytorch.py 95.55% <ø> (ø)
doctr/models/recognition/master/tensorflow.py 96.50% <ø> (+15.12%) ⬆️
doctr/models/recognition/transformer/pytorch.py 100.00% <100.00%> (ø)
doctr/models/recognition/transformer/tensorflow.py 88.11% <100.00%> (+1.02%) ⬆️

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 b2ded17...b5e3b3e. Read the comment docs.

@charlesmindee charlesmindee changed the title feat: add dropout feat: add dropout in MASTER Jul 5, 2021
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 added some questions

doctr/models/recognition/master/tensorflow.py Outdated Show resolved Hide resolved
@@ -239,6 +255,8 @@ def call(
x *= tf.math.sqrt(tf.cast(self.d_model, tf.float32))
x += self.pos_encoding[:, :seq_len, :]

x = self.dropout(x, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Mmmh, we already have dropout in the DecoderLayer, are you positive we're supposed to put one here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://www.tensorflow.org/text/tutorials/transformer#decoder Here they put another one here, but we can remove it

Copy link
Collaborator Author

@charlesmindee charlesmindee Jul 5, 2021

Choose a reason for hiding this comment

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

In pytorch it is also used here after the postional_embedding: https://pytorch.org/tutorials/beginner/transformer_tutorial.html#define-the-model

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! Seems better aligned between PyTorch and TF with your last commits 👌

@charlesmindee charlesmindee merged commit 9f7edcd into main Jul 6, 2021
@charlesmindee charlesmindee deleted the dropout branch July 6, 2021 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework: pytorch Related to PyTorch backend framework: tensorflow Related to TensorFlow backend module: models Related to doctr.models type: enhancement Improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants