Evaluation of existing Poincaré embedding implementations #1643

janpom · 2017-10-23T08:24:04Z

janpom · 2017-10-23T08:27:28Z

docs/notebooks/Poincare Evaluation.ipynb

+   "outputs": [],
+   "source": [
+    "# Set this to the path of the directory that is to contain the poincare-embedding directory\n",
+    "parent_directory = '/home/jayant/projects/'"


Please use path relative to the notebook, so that you don't need to hardcode your home dir path.

janpom · 2017-10-23T08:30:02Z

docs/notebooks/Poincare Evaluation.ipynb

+   ],
+   "source": [
+    "# Download and apply patch\n",
+    "patch_url = \"https://raw.githubusercontent.com/RaRe-Technologies/gensim/poincare_eval/docs/notebooks/poincare_data/poincare_burn_in_eps.patch\"\n",


The patch is in the repo so you can just reference the local file (poincare_data/poincare_burn_in_eps.patch). Besides, the URL will get broken once the code is merged to master.

Good point, changed.

janpom · 2017-10-23T09:02:52Z

docs/notebooks/Poincare Evaluation.ipynb

+     "text": [
+      "Results for WordNet Link Prediction task\n",
+      "+--------------------------------------------+---------------------------------+\n",
+      "|                                            |        Model Dimensions         |\n",


Table header misaligned.

janpom · 2017-10-23T09:27:39Z

docs/notebooks/Poincare Evaluation.ipynb

+    "    \n",
+    "    \n",
+    "    @staticmethod\n",
+    "    def get_positive_relation_ranks(distances, positive_relations):\n",


There's a bit too much magic in this method implementation. Are you sure it actually does what it's expected to do? Have you tested it?

Yeah I agree the code isn't straightforward. I changed distances to all_distances to make it slightly clearer. Not sure how it can be made clearer. And yes, I've tested it -

all_distances = np.array([5, 10, 15, 20, 0, 1, 2, 3]) positive_relations = [4, 6, 1, 7] assert(ReconstructionEvaluation.get_positive_relation_ranks(all_distances, positive_relations) == [1, 2, 3, 2])

My concern wasn't about naming, but rather about the used constructions. Mainly

ranks = (negative_relation_distances < positive_relation_distances[:, np.newaxis]).sum(axis=1) + 1

is pretty non-obvious. I have tested it myself, though, and indeed does seem to work, so that's good.

janpom · 2017-10-23T10:25:57Z

docs/notebooks/Poincare Evaluation.ipynb

+    "                break\n",
+    "        return np.mean(ranks)\n",
+    "    \n",
+    "    def evaluate_map(self, max_n=None):\n",


What's the plan with evaluate_map?

Ah yes, I'd forgotten. Will implement.

menshikh-iv · 2017-10-24T16:37:21Z

@jayantj please ignore CI now, don't worry about it.

janpom · 2017-10-26T07:11:59Z

docs/notebooks/Poincare Evaluation.ipynb

+   "source": [
+    "# Prepare the WordNet data\n",
+    "wordnet_file = os.path.join(data_directory, 'wordnet_noun_hypernyms.tsv')\n",
+    "! python {parent_directory}/poincare-embedding/scripts/create_wordnet_noun_hierarchy.py {wordnet_file}"


Incorrect path: poincare-embedding -> poincare-cpp-embedding

After correcting the path I'm getting:

Traceback (most recent call last): File "/data_ssd/poincare/gensim/docs/notebooks/poincare/poincare-cpp-embedding/scripts/create_wordnet_noun_hierarchy.py", line 4, in <module> from nltk.corpus import wordnet as wn ImportError: No module named nltk.corpus

We may need to add nltk to requirements.

... and click as well.

Thinking more about it, it doesn't seem a great idea to make these gensim requirements just because of the notebook. Let's just do

import nltk import click nltk.download('wordnet')

in the first cell in the notebook so that it fails there if these libs are missing. It should be obvious for the user that they need to install these. (I don't expect non-expert users to ever run the notebook.)

Yeah, makes sense. I added the imports at the beginning, and a link to a separate requirements.txt in poincare/ for easy installation of missing imports.

janpom · 2017-10-26T07:15:32Z

docs/notebooks/Poincare Evaluation.ipynb

+   "cell_type": "markdown",
+   "metadata": {},
+   "source": [
+    "You might need to install an updated version of `cmake` to be able to compile the source code. Please make sure that the binary `poincare-embedding` has been created before proceeding."


Please don't ask the user to do something that's trivial to do automatically. Just check if the file exists and raise an exception if it doesn't. Otherwise, the execution flow continues and fails much later. At that point it's no longer obvious what the root cause is.

janpom · 2017-10-26T07:17:02Z

docs/notebooks/Poincare Evaluation.ipynb

+    "if not os.path.exists(cpp_repo_name):\n",
+    "    ! git clone https://github.com/TatsuyaShirakawa/poincare-embedding.git {cpp_repo_name}\n",
+    "\n",
+    "patches_applied = False"


This looks wrong. If the notebook is re-run with force_setup == False the patches will get applied twice.

…misaligned header

…mprovements

janpom changed the title ~~Evaluation of existing Pointcaré embedding implementations #1624~~ Evaluation of existing Pointcaré embedding implementations Oct 23, 2017

janpom commented Oct 23, 2017

View reviewed changes

jayantj force-pushed the poincare_eval branch from 58fc15e to 0f48f20 Compare October 23, 2017 11:09

jayantj force-pushed the poincare_eval branch from 0c5609c to 0690746 Compare October 25, 2017 06:49

gojomo changed the title ~~Evaluation of existing Pointcaré embedding implementations~~ Evaluation of existing Poincaré embedding implementations Oct 26, 2017

janpom commented Oct 26, 2017

View reviewed changes

jayantj force-pushed the poincare_eval branch from 14060f3 to b7d0095 Compare October 26, 2017 14:15

jayantj added 16 commits October 26, 2017 21:31

Adds initial poincare evaluation notebook

51bd7ab

More efficient computation of mean rank for graph reconstruction

9a511a7

Adds initial evaluation for lexical entailment on HyperLex

0a06fd5

Adds complete optimized evaluation of lexical entailment to notebook

fd86c32

Adds setup and training steps to notebook, tabulated results

69b4d61

Implements link prediction task for poincare and adds results

c17846f

Adds patch for C++ poincare implementation

efd1fe0

Adds patch file to setup and all results

c062814

Minor fixes to poincare nb - change in variable name, relative path, …

1f15aeb

…misaligned header

Adds results for all models on link prediction

a415c65

Adds initial implementation of MAP and MAP scores

0d593f0

Corrects implementation of MAP, updated results

d7f1840

More readable results

53fcf23

Adds code for training and loading external numpy models, results

5d72642

Adds patch for external numply implementation to repo

7cbd6b9

Adds cleaner setup

c07d582

jayantj added 4 commits October 26, 2017 21:31

Adds results of numpy poincare embeddings on link prediction, minor i…

17390ac

…mprovements

Minor fixes to poincare eval notebook

e80a834

Adds poincare nb requirements, moves imports to beginning

1e7ddd8

Doesnt load all models into memory at once

99089a5

jayantj force-pushed the poincare_eval branch from b7d0095 to 99089a5 Compare October 26, 2017 20:37

janpom merged commit 3ea3730 into poincare Oct 30, 2017

janpom deleted the poincare_eval branch October 30, 2017 09:46

janpom mentioned this pull request Oct 30, 2017

Evaluation of existing Poincaré embedding implementations #1624

Closed

jayantj mentioned this pull request Dec 4, 2017

[MRG] Add Poincare model #1757

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Evaluation of existing Poincaré embedding implementations #1643

Evaluation of existing Poincaré embedding implementations #1643

janpom commented Oct 23, 2017 •

edited

Loading

janpom Oct 23, 2017

jayantj Oct 23, 2017

janpom Oct 23, 2017

jayantj Oct 23, 2017

janpom Oct 23, 2017

jayantj Oct 23, 2017

janpom Oct 23, 2017

jayantj Oct 23, 2017 •

edited

Loading

janpom Oct 23, 2017

janpom Oct 23, 2017

jayantj Oct 23, 2017

menshikh-iv commented Oct 24, 2017

janpom Oct 26, 2017

janpom Oct 26, 2017

janpom Oct 26, 2017

janpom Oct 26, 2017 •

edited

Loading

jayantj Oct 26, 2017

janpom Oct 26, 2017

jayantj Oct 26, 2017

janpom Oct 26, 2017 •

edited

Loading

jayantj Oct 26, 2017

Evaluation of existing Poincaré embedding implementations #1643

Evaluation of existing Poincaré embedding implementations #1643

Conversation

janpom commented Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayantj Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

menshikh-iv commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janpom Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janpom Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janpom commented Oct 23, 2017 •

edited

Loading

jayantj Oct 23, 2017 •

edited

Loading

janpom Oct 26, 2017 •

edited

Loading

janpom Oct 26, 2017 •

edited

Loading