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

Patch neg sampling #1796

Merged
merged 2 commits into from
Jun 16, 2021
Merged

Patch neg sampling #1796

merged 2 commits into from
Jun 16, 2021

Conversation

armingh2000
Copy link
Contributor

Description of changes:
For negative sampling, the get_negatives function is not working properly. I changed the way it iterates through the dictionary. It works fine now.

@AnirudhDagar I made 2 commits here. The first is for .md file. The second one is after running d2lbook build lib and nothing else was changed thereafter. But there are no imports in torch.py file :|

By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice.

@AnirudhDagar
Copy link
Member

This has been opened in favour of #1793.
No worries @armingh2000! Thanks for your contributions, I've made the change for rest of it.

Copy link
Member

@AnirudhDagar AnirudhDagar left a comment

Choose a reason for hiding this comment

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

LGTM! Will wait for @astonzhang to merge and double check this.

@armingh2000
Copy link
Contributor Author

@AnirudhDagar Thanks a lot for your patience. Can you say where did I get wrong? Just for further contributions, I am asking.

@mli
Copy link
Member

mli commented Jun 14, 2021

Job d2l-en/PR-1796/2 is complete.
Check the results at http://preview.d2l.ai/d2l-en/PR-1796/

@AnirudhDagar
Copy link
Member

AnirudhDagar commented Jun 15, 2021

@armingh2000 I'm not very sure why the d2lbook build lib command missed imports on your machine. Please make sure that the chapter preface was there in your local repo, since all the imports reside there.

If that doesn't solve the problem, i think it might be a windows only issue, you can go ahead and raise an issue on the d2lbook github repo for that.

@armingh2000
Copy link
Contributor Author

@AnirudhDagar yes I think you're right. I'll check that out. Thanks

@astonzhang
Copy link
Member

@armingh2000 Great catch! I'll further simplify your proposed change as
sampling_weights = [count**0.75 for count in counter.values()]

@astonzhang astonzhang merged commit afeee4d into d2l-ai:master Jun 16, 2021
@astonzhang
Copy link
Member

@armingh2000, would you like to send another PR to replace your id with name in our acknowledgement:
https://github.com/d2l-ai/d2l-en/blob/master/chapter_preface/index.md

Thanks!

@armingh2000
Copy link
Contributor Author

armingh2000 commented Jun 17, 2021

@astonzhang yes that would be better. Should I change the related .md file and compile after that? If not, how can I change my id to my name?
Thanks

@astonzhang
Copy link
Member

@astonzhang yes that would be better. Should I change the related .md file and compile after that? If not, how can I change my id to my name?
Thanks

You may wish to refer to 19.6.1 https://d2l.ai/chapter_appendix-tools-for-deep-learning/contributing.html on how to edit the md file (replacing id with name)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants