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

Add model_to_dict one-liner to word2vec notebook. Fix #1269 #1776

Merged
merged 3 commits into from
Dec 12, 2017

Conversation

kakshay21
Copy link
Contributor

Adding to the word2vec to production pipeline #1269

@kakshay21
Copy link
Contributor Author

I've created a small patch now @gojomo

"metadata": {},
"outputs": [],
"source": [
"def model_to_dict():\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

You no need this function for oneliner in notebook,
most_similars_precalc = {word : model.wv.most_similar(word) for word in model.wv.index2word} enough here

"cell_type": "markdown",
"metadata": {},
"source": [
"### Adding Word2Vec \"model to dict\" method to production pipeline"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add more motivation (why this can be useful, it will be good if you show concrete use-case, how you'll use most_similars_precalc )

@menshikh-iv menshikh-iv changed the title Add model to dict method Add model_to_dict one-liner to word2vec notebook. Fix #1269 Dec 11, 2017
@kakshay21
Copy link
Contributor Author

Any changes required in the documentation? @menshikh-iv

@menshikh-iv
Copy link
Contributor

@kakshay21 concrete example with comparison with/without caching

"### Adding Word2Vec \"model to dict\" method to production pipeline\n",
"Suppose, we still want more performance improvement in production. \n",
"One good way is to cache all the similar words in a dictionary.\n",
"\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

No needed \n after each sentence + you should add a link to the concrete issue in the notebook.

@kakshay21
Copy link
Contributor Author

Is it fine? @menshikh-iv
I've tested it locally but didn't run the notebook to prevent larger diffs

@menshikh-iv
Copy link
Contributor

Thanks @kakshay21, congratz with the first contribution 🥇

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.

2 participants