Fixed KeyError in coherence model

[pietrotrope]

When the token list of a topic contains a token which is not present in the dictionary token2id the framework raised an error instead of excluding the token and continue to compute the metric score.

Excluding the tokens of the topics which are not present in the dictionary allows to compute the metric on different texts that do not have to contain every representative word of the topics.

• Fix Coherence score on new data Key Error #2711
• Fix Coherence key error on held out set  #3111

[piskvorky]

Thanks. Can you add a test too?

Are there other places in the coherence with the same issue? (poor input validation / preprocessing) Let's fix them all at once.

[pietrotrope]

I ran some test and i fail when passing topics id's instead of tokens.
I need to solve this issue, after that we can proceed to search other places with the same issue

[pietrotrope]

The idea behind the issue is to allow valuation of topics with new tokens and/or ID's that are not present in the texts by excluding them from the computation.

To explain i will refer to the issue example with a simple test.

Given:

topics = [  ['human', 'computer', 'system', 'interface'],
            ['graph', 'minors', 'trees', 'eps']]

test_WITH_human = [  ['human', 'computer', 'system', 'interface'],
                     ['graph', 'minors', 'trees', 'eps']]

test_WITHOUT_human = [  [ 'computer', 'system', 'interface'],
                        ['graph', 'minors', 'trees', 'eps']]

The following code

dictionary = gensim.corpora.Dictionary(test_WITHOUT_human)
corpus = [dictionary.doc2bow(text) for text in test_WITHOUT_human]

cm = CoherenceModel(topics=topics, 
                    corpus=corpus, 
                    texts=test_WITHOUT_human,
                    dictionary=dictionary, 
                    coherence="c_npmi")

cm.get_coherence()

Returns

--> 445             return np.array([self.dictionary.token2id[token] for token in topic])
    446         except KeyError:  # might be a list of token ids already, but let's verify all in dict

KeyError: 'human'

Instead of:
1.000000000005771

which is the metric score.

With the last update i get the right result from this test, but it raises some other issues due to the fact that in the initial version, token or id formatting was retrieved by handling an exception.

Original code:

def _ensure_elements_are_ids(self, topic):
        try:
            return np.array([self.dictionary.token2id[token] for token in topic])
        except KeyError:  # might be a list of token ids already, but let's verify all in dict
            topic = (self.dictionary.id2token[_id] for _id in topic)
            return np.array([self.dictionary.token2id[token] for token in topic])

To find out if topics are in token or ID form, this code consider an exception as an hint that topics lists are expressed in ID form and proceed to retrieve them from their ID's

But if we have a topic with a token not present in the texts the code raise the exception and proceed to compute topics as if they're in ID form.
Topics are not in ID form and we got a runtime error.

Let's try to add a control to exclude tokens which are not present in the texts:

def _ensure_elements_are_ids(self, topic):
        try:
            return np.array([self.dictionary.token2id[token] for token in topic if token in self.dictionary.token2id])
        except KeyError:  # might be a list of token ids already, but let's verify all in dict
            topic = (self.dictionary.id2token[_id] for _id in topic if _id in self.dictionary.id2token)
            return np.array([self.dictionary.token2id[token] for token in topic])

The test in the example with tokens is now covered, but if the input is made of ID's and not tokens, the code will not raise an exception because every id is not a key in the token2id dictionary and it will be excluded, resulting in a list of 0 topics instead of an excpetion which trigger the right code.

To handle the problem i proceeded to try this code:

def _ensure_elements_are_ids(self, topic):
           elements_are_tokens = np.array([self.dictionary.token2id[token] for token in topic if token in self.dictionary.token2id])
           topic_tokens_from_id = (self.dictionary.id2token[_id] for _id in topic if _id in self.dictionary.id2token)
           elements_are_ids = np.array([self.dictionary.token2id[token] for token in topic_tokens_from_id])
           if elements_are_tokens.size > elements_are_ids.size:
               return elements_are_tokens
           elif elements_are_ids.size > elements_are_tokens.size:
               return elements_are_ids
           else:
               raise Exception("Topic list is not a list of lists of tokens or ids")

With this code, we assume that the right formatting is the one with the higher number of matches with the token2id or id2token dictionaries.
When we can't say if the right formatting is tokens lists or ID's lists we raise an exception.
This example seems to pass more tests but it makes an assumption on the data formatting instead of finding out the real one.

If anyone can help me to solve the problem i will gladly try to work on it again and solve the problem once and for all.

[mpenkov]

Picked up a code formatting nitpick.

Is this code covered by tests? If not, please add them.

[mpenkov]

Why did you add this blank line?

[mpenkov]

To explain i will refer to the issue example with a simple test.

I think this would make a good unit test. It would demonstrate the usefulness of your fix and prevent future regressions.

[piskvorky]

Looks like we dropped the ball on this PR. People still keep tripping over the same issue.

@pietrotrope @mpenkov do you think we could finish this & merge? Thanks.

[pietrotrope]

hi, I tried to update the code by modifying it according to the requests.
I point out that the solution that I have proposed in recent months (and that I am trying to submit with this commit) excludes from the calculation of the coherence the words that are not present in the used dictionary.

[piskvorky]

Thanks @pietrotrope .

I point out that the solution that I have proposed in recent months (and that I am trying to submit with this commit) excludes from the calculation of the coherence the words that are not present in the used dictionary.

That's fine – better than an exception at any rate. Is this fact clearly documented?

@mpenkov can you please review & merge?

[T0admomo]

pietrotrope - just wanted to say thank you for taking the time to propose this fix! New to gensim and I quickly encountered this problem, this fix will cut down on a lot of preprocessing! following closely !

[mpenkov]

Left some comments, please have a look. Sorry it took me so long to review this.

[pietrotrope]

Hi! I updated the code,
as i said in the comments in the revision i did some refactoring (following your suggestions) and added the new tests (also corrected one)

[mpenkov]

Can you merge origin/develop branch into your PR's branch? Looks like there's some kind of conflict.

[mpenkov]

Note to self: resolve conflicts, check tests.

[mpenkov]

Merged. Thank you @pietrotrope !