Skip to content

Conversation

Yashashree304
Copy link
Contributor

Copy link
Member

@MarkDaoust MarkDaoust left a comment

Choose a reason for hiding this comment

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

Can you keep the original URL:

https://tensorflow.org/tutorials/text/images/word2vec_negative_sampling.png

And replace the old image at images/word2vec_negative_sampling.png

Also please be sure you're using an actual PNG file, not a JPEG. JPEG is not a good format for diagrams.

@Yashashree304
Copy link
Contributor Author

Made the necessary changes as told by you. However, the old file on the link is visible in the notebook, not the new file, I guess this is because my image is not hosted on the TensorFlow website.

@MarkDaoust
Copy link
Member

Thanks.

It doesn't show up yet, but after submission this will eventually get synced to tensorflow.org, and then the changes will be visible in the doc.

I could have been more clear about that.

@Yashashree304
Copy link
Contributor Author

@MarkDaoust I have already done the changes as told by you. Do I need to make any more changes as I can see some changes are requested in the PR.

MarkDaoust
MarkDaoust previously approved these changes Jan 21, 2022
@MarkDaoust MarkDaoust added the ready to pull Start merge process label Jan 21, 2022
@github-actions github-actions bot added the lgtm Community-added approval label Jan 21, 2022
@MarkDaoust
Copy link
Member

I think Everything's okay here. Let me see if I can merge it.

@8bitmp3
Copy link
Contributor

8bitmp3 commented Jan 24, 2022

Thanks @Yashashree304 @MarkDaoust On it (import/copybara)

@8bitmp3 8bitmp3 removed ready to pull Start merge process lgtm Community-added approval labels Jan 24, 2022
@8bitmp3
Copy link
Contributor

8bitmp3 commented Jan 24, 2022

@Yashashree304 There may be some issues with the grammar/syntax here:

image

Since the input sentence is "The wide road shimmered in the hot sun", perhaps the following may be more straightforward:

[Updated]

Note: Words `temperature` and `code` are not part of the input sentence. They
belong to the vocabulary like certain other indices used in the diagram above.

(Notice the back ticks.)

[Update] Also, as per @markmcd 's suggestion, let's place the text outside the image so that it's more accessible and easier to maintain.

Since the remaining indices that you referred to in the last sentence do belong to the words from the input sentence, "similarly..." may not necessary/appropriate in this context.

LMKWYT

@8bitmp3 8bitmp3 added lgtm Community-added approval ready to pull Start merge process labels Jan 24, 2022
@copybara-service copybara-service bot merged commit 4e18720 into tensorflow:master Jan 24, 2022
@MarkDaoust
Copy link
Member

That is an improvement, thanks @8bitmp3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Community-added approval ready to pull Start merge process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect figure on word2vec tutorial
3 participants