Adding common terms to phrases model

[alexgarel]

This is an implementation for #1258 (and a replacement to aborted PR #1263).

I've tested different implementation. This one in my view is effective and helps code maintenability.

[alexgarel]

@gojomo did you want to review this ?

[alexgarel]

@piskvorky you proposed me once your help on #1263. This is the corresponding new PR. It has been stalled yet.
No hurry, you maybe focused on other topics, but just wanted you to know it exists :-)

[piskvorky]

Thanks for the PR @alexgarel ! @menshikh-iv will review soon, sorry for the long wait, we were busy refactoring.

[menshikh-iv]

Looks very good for me, big thanks @alexgarel 🔥, please add explicit inheritance and I'll merge your PR

[menshikh-iv]

Inherit object explicitly

[menshikh-iv]

You create a new class for avoiding copy-paste in two child classes, I'm correct?

[alexgarel]

Yes that's it.

[alexgarel]

@menshikh-iv I did the requested changes.

Appveyor did not pass, but for a strange reason that seems uncorrelated…

[menshikh-iv]

Nice @alexgarel, please resolve merge conflict and we'll merge this PR.

[menshikh-iv]

@alexgarel please ping me when you'll be ready to review.

[alexgarel]

@menshikh-iv I finally did it :-)

[menshikh-iv]

Very nice feature, big thanks @alexgarel 🔥 🥇 Contgrazt with first contribution 👍

[menshikh-iv]

Hi @alexgarel, we think that this feature is very nice, for this reason, we want to write the blogpost about it. Can you help with blogpost (or even write yourself)?

[piskvorky]

@alexgarel even a short summary would be useful, to start with: what is this new functionality for, what can be done now that couldn't be done before, how to use it properly.

We'll include your description in the release notes, to give users motivation to try it out and use it :)

[alexgarel]

Hello, thanks a lot for the encouragement. Time is quite a problem for me in this very moment, what is the schedule ?

[piskvorky]

Summary and motivation "for dummies": 1-3 paragraphs, as soon as you can.

Blog post: at your leisure; do you think you could have a draft by the end of November?

[alexgarel]

Ok, I'll try to do my best :-)

[alexgarel]

@piskvorky I though the Summary and motivation "for dummies" was meant to be in release notes… but release note is already published, isn't it ?

However here is a short description:

Phrases now as a common_terms optional keyword arguments. You can pass it a list of words. Those words will have a special behaviour : their presence between two words won't prevent bigram detection but they will be kept if the resulting expression has a high enough score. Frequency of those common terms alone are not taken into account when computing the score of the expression.

It offers an alternative to common terms (aka stop words) removal before bigram detection, allowing to keep the information they carry. It may for example capture « eye of the beholder » or « code of conduct » and also distinguish « car with driver » from « car without driver ».

[piskvorky]

It is already published, but can be edited :) The summary is also great for the module docstrings.

Thanks a lot!

[dldx]

I have a Phrases model that was computed using Gensim 2.2.0 but because of these changes, I cannot load it anymore. This is the error I get:

---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<timed exec> in <module>()

~/Stuff/Sources/anaconda3/envs/nlp/lib/python3.6/site-packages/gensim/models/phrases.py in __init__(self, phrases_model)
    546         self.delimiter = phrases_model.delimiter
    547         self.scoring = phrases_model.scoring
--> 548         self.common_terms = phrases_model.common_terms
    549         corpus = self.pseudocorpus(phrases_model)
    550         self.phrasegrams = {}

AttributeError: 'Phrases' object has no attribute 'common_terms'

Any suggestions on what I can do to fix this? :)

Cheers!

[piskvorky]

@menshikh-iv was this tested for backward compatibility?

I think this could be easy (using empty common_terms on load if it's missing, or something like that), but we definitely need that.

Sorry @dldx , bug fix coming soon :) Can you open a new issue for this?

[dldx]

@piskvorky #1751

Thanks for developing Gensim! It's great!