Add `common_terms` parameter to `sklearn_api.PhrasesTransformer` #2074

pmlk · 2018-05-30T11:20:39Z

make use of common_terms parameter added to underlying models.phrases.Phrases class in commit b4515e0
utilize models.phrases.Phraser to avoid warning:

UserWarning: For a faster implementation, use the gensim.models.phrases.Phraser class

This parameter is being propagated to the underlying models.Phrases class.

this avoids the following warning: "UserWarning: For a faster implementation, use the gensim.models.phrases.Phraser class"

menshikh-iv

Thanks @pmlk, in general, looks good, only several comments, please fix it and I'll merge PR

gensim/test/test_sklearn_api.py

menshikh-iv · 2018-07-30T13:01:25Z

gensim/test/test_sklearn_api.py

@@ -1151,6 +1159,58 @@ def testModelNotFitted(self):
        self.assertRaises(NotFittedError, phrases_transformer.transform, phrases_sentences[0])


+class TestPhrasesTransformerCommonTerms(unittest.TestCase):
+    def setUp(self):
+        numpy.random.seed(0)


That's not good, because this freeze "global" seed (effect to all test cases, not only to your class). Why you need this here? Probably you can simply remove this line.

Quite honestly, I was just looking at the other test cases. Almost all of them include that line in setUp. But I'll remove it. :-)

oh, you are right, that's looks like a bug for me

ivan@P50:~/release/gensim$ find gensim/ -name "*.py" | xargs grep "numpy.random.seed" gensim/test/test_sklearn_api.py: numpy.random.seed(0) # set fixed seed to get similar values everytime gensim/test/test_sklearn_api.py: id2word=dictionary, num_topics=2, passes=100, minimum_probability=0, random_state=numpy.random.seed(0) gensim/test/test_sklearn_api.py: id2word=dictionary, num_topics=10, passes=100, minimum_probability=0, random_state=numpy.random.seed(0) gensim/test/test_sklearn_api.py: minimum_probability=0, random_state=numpy.random.seed(0) gensim/test/test_sklearn_api.py: numpy.random.seed(0) # set fixed seed to get similar values everytime gensim/test/test_sklearn_api.py: model = LdaTransformer(num_topics=2, passes=10, minimum_probability=0, random_state=numpy.random.seed(0)) gensim/test/test_sklearn_api.py: minimum_probability=0, random_state=numpy.random.seed(0) gensim/test/test_sklearn_api.py: numpy.random.seed(0) # set fixed seed to get similar values everytime gensim/test/test_sklearn_api.py: numpy.random.seed(0) # set fixed seed to get similar values everytime gensim/test/test_sklearn_api.py: numpy.random.seed(13) gensim/test/test_sklearn_api.py: numpy.random.seed(0) # set fixed seed to get similar values everytime gensim/test/test_sklearn_api.py: numpy.random.seed(0) gensim/test/test_sklearn_api.py: numpy.random.seed(0) # set fixed seed to get similar values everytime gensim/test/test_sklearn_api.py: numpy.random.seed(0) gensim/test/test_sklearn_api.py: numpy.random.seed(0) # set fixed seed to get similar values everytime gensim/test/test_sklearn_api.py: numpy.random.seed(0) gensim/test/test_sklearn_api.py: lda_model = LdaTransformer(num_topics=2, passes=10, minimum_probability=0, random_state=numpy.random.seed(0)) gensim/test/test_sklearn_api.py: numpy.random.seed(0) gensim/test/test_sklearn_api.py: lda_model = LdaTransformer(num_topics=2, passes=10, minimum_probability=0, random_state=numpy.random.seed(0)) gensim/test/test_sklearn_api.py: numpy.random.seed(0) gensim/test/test_sklearn_api.py: numpy.random.seed(0) gensim/test/test_sklearn_api.py: numpy.random.seed(0)

that's not your fault, please simply remove this line from your code, that's enough.

menshikh-iv · 2018-08-14T01:49:58Z

gensim/test/test_sklearn_api.py

+
+    def testCompareToOld(self):
+        # Phrases-model extracted from PhrasesTransformer fitted same way as in above test class TestPhrasesTransformer
+        phrases_model = models.phrases.Phrases.load("gensim/test/test_data/phrases_for_phrases_transformer.model")


Instead of gensim/test/test_data/phrases_for_phrases_transformer.model

from gensim.test.utils import datapath datapath("phrases_for_phrases_transformer.model")

BTW, you need to load old PhrasesTransformer, not models.phrases.Phrases, you shouldn't construct distinct objects and assign it to attributes manually in test.

menshikh-iv · 2018-08-14T02:11:41Z

gensim/test/test_sklearn_api.py

+        # Phrases-model extracted from PhrasesTransformer fitted same way as in above test class TestPhrasesTransformer
+        phrases_model = models.phrases.Phrases.load("gensim/test/test_data/phrases_for_phrases_transformer.model")
+        old_phrases_transformer = PhrasesTransformer(min_count=1, threshold=1)
+        # manually set models instead of using fit()


menshikh-iv · 2018-08-14T02:14:39Z

gensim/sklearn_api/phrases.py

@@ -131,15 +138,16 @@ def transform(self, docs):
            Phrase representation for each of the input sentences.

        """
-        if self.gensim_model is None:
+        if self.gensim_model is None or self.phraser is None:


That's not true too: if you load old model - self.gensim_model exist, but self.phraser isn't (through model was fitted early).

gensim/sklearn_api/phrases.py

A pre-trained Phrases model (self.gensim_model) may be set to avoid using the fit() method. In transform(), the also necessary Phraser model (self.phraser) will be instantiated if it hasn't been before.

pmlk · 2018-08-14T09:27:10Z

gensim/test/test_sklearn_api.py

@@ -1170,7 +1170,8 @@ def setUp(self):
        ]

    def testCompareToOld(self):
-        old_phrases_transformer = pickle.load(datapath("phrases_transformer.pkl"))
+        with open(datapath("phrases_transformer.pkl"), "rb") as old_phrases_transformer_pkl:
+            old_phrases_transformer = pickle.load(old_phrases_transformer_pkl)
        doc = phrases_sentences[-1]
        phrase_tokens = old_phrases_transformer.transform(doc)[0]


The old PhrasesTransformer didn't have the self.phraser attribute which is checked in the new transform() method. This causes this test to fail (at least on my local machine). It seems that the new transform() method is being called here instead of the old one. Do I need to take extra care of object methods being pickled correctly?

" old one" no more exists, you have an old model (like a key-value store of data) and new code.

Do I need to take extra care of object methods being pickled correctly?

Of course, see https://docs.python.org/2/library/pickle.html#pickling-and-unpickling-normal-class-instances

menshikh-iv

I think you need to write custom __setstate__ method for fix backward compatibility issue, see https://docs.python.org/2/library/pickle.html#pickling-and-unpickling-normal-class-instances

gensim/test/test_sklearn_api.py

menshikh-iv · 2018-08-27T06:16:26Z

ping @pmlk, are you planning to finish PR?

pmlk · 2018-08-27T07:57:37Z

I would like to. However, I am still not sure how to pickle instance methods (the referenced docs aren't really helpful to me).

menshikh-iv · 2018-08-29T03:12:17Z

@pmlk why you need to pickle method? You just need to add workaround for phraser and common_terms: If old version of object loaded (without this parameters - fill this parameter in __setstate__). Also see good examples of __setstate__ & __getstate__ from https://stackoverflow.com/questions/1939058/simple-example-of-use-of-setstate-and-getstate

pmlk · 2018-09-15T09:52:43Z

ping @menshikh-iv, any more changes necessary? :-)

menshikh-iv · 2018-09-24T05:36:18Z

Sorry for waiting @pmlk,

need to add the test with loading old model, see #2074 (comment) and test for new model serialized / deserialized correctly (i.e save & load with pickle)

pmlk · 2018-09-24T08:41:44Z

gensim/test/test_sklearn_api.py

+            [u'the', u'bank_of_america', u'offices', u'are', u'closed']
+        ]
+
+    def testCompareToOld(self):


need to add the test with loading old model

just to clarify, @menshikh-iv: this test loads an old model, so I would assume I would only need to add another test for de-/serializing the new model as per your comment

test for new model serialized / deserialized correctly (i.e save & load with pickle)

correct me if I'm wrong

yes, you are correct.

menshikh-iv · 2018-10-04T12:17:17Z

Thanks @pmlk, congratz with first contribution 🥇

pmlk added 3 commits May 29, 2018 22:42

add common_terms parameter

a5518e9

This parameter is being propagated to the underlying models.Phrases class.

add tests for new common_terms parameter

2db1a25

utilize models.phrases.Phraser class

cbe6061

this avoids the following warning: "UserWarning: For a faster implementation, use the gensim.models.phrases.Phraser class"

menshikh-iv suggested changes Jul 30, 2018

View reviewed changes

add testCompareToOld, add pre-trained Phrases model

35ab612

menshikh-iv suggested changes Aug 14, 2018

View reviewed changes

pmlk added 3 commits August 14, 2018 10:51

use pickle to load old PhrasesTransformer

882a0f4

allow setting Phrases model without setting Phraser model

e7b01cd

A pre-trained Phrases model (self.gensim_model) may be set to avoid using the fit() method. In transform(), the also necessary Phraser model (self.phraser) will be instantiated if it hasn't been before.

open pickle file

86f3d57

pmlk commented Aug 14, 2018

View reviewed changes

menshikh-iv suggested changes Aug 15, 2018

View reviewed changes

gensim/test/test_sklearn_api.py Outdated Show resolved Hide resolved

pmlk added 2 commits September 6, 2018 00:01

add __setstate__ for backward compatibility

c0b4a95

use pickle protocol 2

f3fcfb7

pmlk commented Sep 24, 2018

View reviewed changes

test loading new phrases transformer

41c21a7

menshikh-iv changed the title ~~Propagate common_terms parameter to sklearn_api PhrasesTransformer~~ Add common_terms parameter to sklearn_api.PhrasesTransformer Oct 4, 2018

menshikh-iv approved these changes Oct 4, 2018

View reviewed changes

menshikh-iv merged commit 367bdbd into piskvorky:develop Oct 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `common_terms` parameter to `sklearn_api.PhrasesTransformer` #2074

Add `common_terms` parameter to `sklearn_api.PhrasesTransformer` #2074

pmlk commented May 30, 2018 •

edited

Loading

menshikh-iv left a comment

menshikh-iv Jul 30, 2018 •

edited

Loading

pmlk Jul 30, 2018

menshikh-iv Jul 31, 2018

menshikh-iv Aug 14, 2018

menshikh-iv Aug 14, 2018

menshikh-iv Aug 14, 2018

menshikh-iv Aug 14, 2018

pmlk Aug 14, 2018 •

edited

Loading

menshikh-iv Aug 15, 2018

menshikh-iv left a comment

menshikh-iv commented Aug 27, 2018

pmlk commented Aug 27, 2018

menshikh-iv commented Aug 29, 2018

pmlk commented Sep 15, 2018

menshikh-iv commented Sep 24, 2018

pmlk Sep 24, 2018

menshikh-iv Sep 25, 2018

menshikh-iv commented Oct 4, 2018

Add common_terms parameter to sklearn_api.PhrasesTransformer #2074

Add common_terms parameter to sklearn_api.PhrasesTransformer #2074

Conversation

pmlk commented May 30, 2018 • edited Loading

menshikh-iv left a comment

Choose a reason for hiding this comment

menshikh-iv Jul 30, 2018 • 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

pmlk Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

menshikh-iv left a comment

Choose a reason for hiding this comment

menshikh-iv commented Aug 27, 2018

pmlk commented Aug 27, 2018

menshikh-iv commented Aug 29, 2018

pmlk commented Sep 15, 2018

menshikh-iv commented Sep 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

menshikh-iv commented Oct 4, 2018

Add `common_terms` parameter to `sklearn_api.PhrasesTransformer` #2074

Add `common_terms` parameter to `sklearn_api.PhrasesTransformer` #2074

pmlk commented May 30, 2018 •

edited

Loading

menshikh-iv Jul 30, 2018 •

edited

Loading

pmlk Aug 14, 2018 •

edited

Loading