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

Fix notebooks #1893

Closed
wants to merge 5 commits into from
Closed

Fix notebooks #1893

wants to merge 5 commits into from

Conversation

sharanry
Copy link
Contributor

As discussed in #1812

@sharanry sharanry changed the title Fix Tensorboard Visualizations notebook Fix notebooks Feb 10, 2018
@menshikh-iv
Copy link
Contributor

@sharanry what's order of merge for this PR (should we merge this before #1812 or otherwise)?

@@ -933,106 +933,6 @@
" <th>100</th>\n",
" <th>200</th>\n",
" </tr>\n",
" <tr>\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again I wasn't able to complete the execution of this cell as it was taking too long

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't good, this information must be in this notebook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay i will try running them again

" <th colspan=\"1\"></th>\n",
" <th colspan=\"6\" style=\"text-align:center\"> Dimensions</th>\n",
" </tr>\n",
" <tr>\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why diff here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't done manually, when i ran the notebook in the docker container the output might have been rendered differently.

@@ -1340,106 +1240,6 @@
" <th>100</th>\n",
" <th>200</th>\n",
" </tr>\n",
" <tr>\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this removed (and exactly same question for similar blocks after).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again this wasnt removed intentionally... the model was taking too long to run on my local system. I ended up just test running some of these cells(they are all working) and interrupting between. So the output is not present.

Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as before #1893 (comment)

@@ -57,7 +57,7 @@ def word2vec2tensor(word2vec_model_path, tensor_filename, binary=False):
outfiletsvmeta = tensor_filename + '_metadata.tsv'

with open(outfiletsv, 'w+') as file_vector:
with open(outfiletsvmeta, 'w+') as file_metadata:
with open(outfiletsvmeta, 'wb+') as file_metadata:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this need? python3.5 problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

%run ../../gensim/scripts/word2vec2tensor.py -i doc_tensor.w2v -o movie_plot This line of code in Tensorboard_visualizations.ipynb fails without this change

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you check this script with other python versions (2.7 or 3.6)? This is really suspicious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay

@sharanry
Copy link
Contributor Author

sharanry commented Feb 13, 2018

@sharanry what's order of merge for this PR (should we merge this before #1812 or otherwise)?

We need to merge this after that one, we may have to resolve one or two merge conflicts while merging this

@menshikh-iv
Copy link
Contributor

Please move current commits to #1812 (see #1812 (comment))

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