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

Revise wrong concatenation at the positional encoding #891

Conversation

@juheeuu
Copy link
Contributor

commented Jul 31, 2019

I think the positional encoding concatenation part is wrong, because it makes the positional encoding vector as like this,
PE_pos = [ sin() ,...,sin(),cos(),...cos()]
But It should be looks like this.
image

Thus, I changed the code.
Please take care :)

@juheeuu juheeuu requested review from brilee, lamberta and MarkDaoust as code owners Jul 31, 2019

@tfdocsbot

This comment has been minimized.

Copy link
Collaborator

commented Jul 31, 2019

Preview and run these notebook edits with Google Colab:

Notebook diffs available on ReviewNB.com.
@googlebot

This comment has been minimized.

Copy link

commented Jul 31, 2019

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no label Jul 31, 2019

@@ -778,7 +805,7 @@
"id": "JPmbr6F1C-v_"
},
"source": [
"Each multi-head attention block gets three inputs; Q (query), K (key), V (value). These are put through linear (Dense) layers and split up into multiple heads. \n",
"![대체 텍스트](https://)Each multi-head attention block gets three inputs; Q (query), K (key), V (value). These are put through linear (Dense) layers and split up into multiple heads. \n",

This comment has been minimized.

Copy link
@lamberta

lamberta Jul 31, 2019

Member

remove

This comment has been minimized.

Copy link
@juheeuu

juheeuu Aug 1, 2019

Author Contributor

I've removed it!

@lamberta lamberta requested a review from yashk2810 Jul 31, 2019

@googlebot

This comment has been minimized.

Copy link

commented Aug 1, 2019

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@yashk2810

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

I don't think it really matters. Can you share the accuracy, loss and prediction changes that you got after doing this?

juheeuu added some commits Jul 31, 2019

@juheeuu juheeuu force-pushed the juheeuu:transformer-positional-encoding branch from a7dcad2 to e6589f0 Aug 1, 2019

@googlebot

This comment has been minimized.

Copy link

commented Aug 1, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Aug 1, 2019

@juheeuu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

@yashk2810
I agree with you, It doesn't affect the result.
The Loss & Accuracy is..
original : Epoch 20 Loss 0.5687 Accuracy 0.3424
changed: Epoch 20 Loss 0.5577 Accuracy 0.3422

But I still think it has to be changed because the code is different with description.
It can be confusing for first-time students like me.
Therefore, I think the code should be modified or a more detailed description added.

@yashk2810

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

LGTM

@lamberta

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

Thanks!

@TensorFlow-Docs-Copybara TensorFlow-Docs-Copybara merged commit e6589f0 into tensorflow:master Aug 1, 2019

2 of 3 checks passed

Ubuntu Sanity Check Internal CI build failed
Details
cla/google All necessary CLAs are signed
import/copybara Change imported to the internal review system
Details

TensorFlow-Docs-Copybara pushed a commit that referenced this pull request Aug 1, 2019

Copybara-Service

@juheeuu juheeuu deleted the juheeuu:transformer-positional-encoding branch Aug 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.