Creating unified base class for all topic models, #938 #946

markroxor · 2016-10-13T07:22:52Z

Attempt to fix #938. Waiting for moderator review to proceed with further commits.

tmylk

It is definitely a step in the right direction.
Please add tests for the print_topic method and changelog.

tmylk · 2016-10-13T13:29:06Z

gensim/models/base.py

@@ -0,0 +1,10 @@
+class BaseModel():


BaseTopicModel is more appropriate

tmylk · 2016-10-13T14:11:40Z

Please rebase off develop and add HDPModel

markroxor · 2016-10-13T14:29:22Z

print_topic is not present in HDPModel. Should I create one like that in LDAModel and LSIModel?

markroxor · 2016-10-13T18:49:02Z

I was not sure about what tests to add, please take a close look on the test files.

tmylk

New testclass requested.

tmylk · 2016-10-14T04:28:49Z

CHANGELOG.md

@@ -10,10 +10,11 @@ Changes
 * Change export_phrases in Phrases model. Fix issue #794 (@AadityaJ,
 [#879](https://github.com/RaRe-Technologies/gensim/pull/879))
    - bigram construction can now support multiple bigrams within one sentence
-* Fixed issue #838, RuntimeWarning: overflow encountered in exp  (@markroxor, [#895](https://github.com/RaRe-Technologies/gensim/pull/895))


why these changes?
Expect to see just 1 line added about the new fix.

I was fixing my previous hyperlinks, but alright. I'll add just one line no issues.

Fixes to your prev links are ok. You can leave them in.

tmylk · 2016-10-14T04:32:04Z

gensim/test/test_ldamodel.py

@@ -253,6 +255,12 @@ def testShowTopics(self):
                self.assertTrue(isinstance(k, six.string_types))
                self.assertTrue(isinstance(v, float))

+        #testing print_topic


create new test test_print_topic in new class TestBaseTopicModel.
TestLdaModel, testLSI and testHDP should inherit from TestBaseTopicModel.

Are the tests okay (the assertions of variable type)?Should I also create a unified test_basemodel.py to handle common tests in all the models like testShowTopic and testShowTopics?

tmylk · 2016-10-14T06:05:30Z

BTW HdpTopicFormatter and hdpmdodel has the print_topics method

devashishd12 · 2016-10-14T10:08:38Z

gensim/models/ldamodel.py

@@ -193,7 +195,7 @@ def get_Elogbeta(self):
 # endclass LdaState


-class LdaModel(interfaces.TransformationABC):
+class LdaModel(interfaces.TransformationABC,basemodel.BaseTopicModel):


slight code style issue. Should be interfaces.TransformationABC, basemodel.BaseTopicModel.

devashishd12 · 2016-10-14T10:11:22Z

gensim/models/lsimodel.py

@@ -221,7 +223,7 @@ def merge(self, other, decay=1.0):
 #endclass Projection


-class LsiModel(interfaces.TransformationABC):
+class LsiModel(interfaces.TransformationABC,basemodel.BaseTopicModel):


same here. You should probably consider setting up a flake8 checks plugin in your editor. Will help you eliminate those nitpicks.

Thanks for the suggestions. I will keep that in mind.

created unified TestBaseModel and aded test for printTopic

tmylk

Almost there! See inline comments

tmylk · 2016-10-16T06:13:00Z

gensim/models/hdpmodel.py

@@ -575,9 +570,6 @@ def __init__(self, dictionary=None, topic_data=None, topic_file=None, style=None

        self.style = style

-    def print_topics(self, num_topics=10, num_words=10):


why is this method removed? it is not inherited from anywhere.
It should remain

It is inherited from the BaseTopicModel class just like lsimodel and ldamodel.

this method is in class HDPFormatter, not HDP

okay I will restore it.

It is still deleted.

Please revert all changes to HdpTopicFormatter

since I've already altered the HdpTopicFormatter class. To unify the model should I remove it's print_topic method and inherit it from its parent class basemodel instead?

tmylk · 2016-10-16T07:40:34Z

gensim/test/test_hdpmodel.py

@@ -47,7 +47,6 @@ def testfile():
    return os.path.join(tempfile.gettempdir(), 'gensim_models.tst')


-
 class TestHdpModel(unittest.TestCase):


should it inherit from TestBaseTopicModel too? Feel free to add the missing methods

tmylk · 2016-10-17T12:41:12Z

gensim/models/hdpmodel.py

@@ -612,10 +604,9 @@ def show_topic_terms(self, topic_data, num_words):
    def format_topic(self, topic_id, topic_terms):
        if self.STYLE_GENSIM == self.style:
            fmt = ' + '.join(['%.3f*%s' % (weight, word) for (word, weight) in topic_terms])
-            fmt = 'topic %i: %s' % (topic_id, fmt)


please revert

Please revert all changes to HdpTopicFormatter

tmylk · 2016-10-17T12:41:25Z

gensim/models/hdpmodel.py

        else:
            fmt = '\n'.join(['    %20s    %.8f' % (word, weight) for (word, weight) in topic_terms])
-            fmt = 'topic %i:\n%s' % (topic_id, fmt)


please revert

This is for passing the testShowTopics test, all the other models throw data in this format. Should I exclude this test too?

it is a different class

what about testShowTopics test? should I override this too for hdpmodel?

Please revert all changes to HdpTopicFormatter

but the Travis build will fail. I have added all the other changes but cannot commit until the tests pass on my local machine.

what tests fail on your local machine?

testPrintTopic and testPrintTopics since the return type of show_topics in hdpmodel does not corresponds to that of ldamodel and lsimodel. To create a unified basemodel the return type should be same so that the unified tests can pass.

tmylk · 2016-10-17T12:44:08Z

Please remove other people's commits from this PR

tmylk · 2016-10-17T12:46:00Z

gensim/test/test_basemodel.py

+    def testShowTopic(self):
+        # TODO create show_topic in HdpModel and then test
+
+        if self.__class__.__name__ == "TestHdpModel":


please override testShowTopic inside TestHdpModel and skip it there

please remove this if statement

tmylk · 2016-10-18T05:28:00Z

gensim/test/test_hdpmodel.py

@@ -47,24 +48,12 @@ def testfile():
    return os.path.join(tempfile.gettempdir(), 'gensim_models.tst')


-
-class TestHdpModel(unittest.TestCase):
+class TestHdpModel(unittest.TestCase, test_basemodel.TestBaseTopicModel):
    def setUp(self):
        self.corpus = mmcorpus.MmCorpus(datapath('testcorpus.mm'))
        self.class_ = hdpmodel.HdpModel
        self.model = self.class_(corpus, id2word=dictionary)



please add the override here

markroxor · 2016-10-18T12:19:27Z

Passed finally!!

piskvorky · 2016-10-20T04:45:31Z

gensim/models/basemodel.py

@@ -0,0 +1,16 @@
+class BaseTopicModel():


All Python classes must inherit from object (new-style classes).

piskvorky · 2016-10-20T04:46:33Z

gensim/models/hdpmodel.py


+        fmt = (topic_id,fmt)


PEP8: space after comma.

piskvorky · 2016-10-20T04:47:13Z

gensim/test/test_basemodel.py

+
+import six
+
+class TestBaseTopicModel():


All tests should inherit from unittest.TestCase (or at least object, if you don't want this to be a real test).

I don't want it to be a real test so adding object instead.

tmylk · 2016-10-20T10:37:41Z

Hi @markroxor There is a problem with the new tests on windows. It is blocking the release. See https://ci.appveyor.com/project/piskvorky/gensim-431bq/build/job/216bwdmb2gqwkme0

markroxor · 2016-10-20T18:08:13Z

Sent PR #969

markroxor changed the title ~~testing base model class~~ testing base model class,fix #938 Oct 13, 2016

markroxor mentioned this pull request Oct 13, 2016

Create single API/base class for all topic models #938

Closed

tmylk suggested changes Oct 13, 2016

View reviewed changes

gensim/models/base.py

@@ -0,0 +1,10 @@

class BaseModel():

Copy link

Contributor

tmylk Oct 13, 2016

Choose a reason for hiding this comment

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

BaseTopicModel is more appropriate

markroxor force-pushed the singleapi branch from 55d8f8d to c25d636 Compare October 13, 2016 15:35

rebased

05edaf2

markroxor force-pushed the singleapi branch from c25d636 to 05edaf2 Compare October 13, 2016 15:43

markroxor added 6 commits October 13, 2016 21:41

tests added for lsi and lda

48396c1

tests added for lsi and lda

e673305

tests added for lsi and lda

de80720

tests added for lsi and lda

272d7fc

tests added for lsi and lda

f4f92d3

registered to CHANGELOG

f9d28a0

markroxor changed the title ~~testing base model class,fix #938~~ Creating unified base class for all topic models, #938 Oct 13, 2016

tmylk suggested changes Oct 14, 2016

View reviewed changes

devashishd12 reviewed Oct 14, 2016

View reviewed changes

markroxor added 2 commits October 15, 2016 11:47

added print_topics() to baseModel

0b47408

added tests for lsi lda, test base

ac4e93b

created unified TestBaseModel and aded test for printTopic

tmylk suggested changes Oct 16, 2016

View reviewed changes

tmylk reviewed Oct 17, 2016

View reviewed changes

markroxor force-pushed the singleapi branch from e37aa81 to ac4e93b Compare October 17, 2016 17:05

tmylk reviewed Oct 18, 2016

View reviewed changes

markroxor force-pushed the singleapi branch 2 times, most recently from 32b7f58 to 096faa5 Compare October 18, 2016 05:59

unified test_hdpmodel

098be5f

markroxor force-pushed the singleapi branch from 096faa5 to 098be5f Compare October 18, 2016 06:01

markroxor added 2 commits October 18, 2016 14:02

changed hdpformatter.show_topics behaviour

a215aed

changed hdpformatter.show_topics behaviour

6de2121

tmylk mentioned this pull request Oct 18, 2016

Notation Mismatch : Documentation LDA and HDP #941

Closed

tmylk merged commit bf7c0ed into piskvorky:develop Oct 18, 2016

piskvorky reviewed Oct 20, 2016

View reviewed changes

markroxor mentioned this pull request Oct 20, 2016

fixed minor formatting and style issues #964

Merged

markroxor mentioned this pull request Oct 20, 2016

fixing appveyor build issue #969

Merged

tmylk mentioned this pull request Nov 8, 2016

Inherit LDAMallet wrapper from BaseModel to standardise print_topics #995

Merged

		@@ -575,9 +570,6 @@ def __init__(self, dictionary=None, topic_data=None, topic_file=None, style=None

		self.style = style

		def print_topics(self, num_topics=10, num_words=10):

		@@ -47,7 +47,6 @@ def testfile():
		return os.path.join(tempfile.gettempdir(), 'gensim_models.tst')



		class TestHdpModel(unittest.TestCase):

Creating unified base class for all topic models, #938 #946

Creating unified base class for all topic models, #938 #946

Conversation

markroxor commented Oct 13, 2016 • edited Loading

tmylk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmylk commented Oct 13, 2016

markroxor commented Oct 13, 2016

markroxor commented Oct 13, 2016

tmylk left a 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

markroxor Oct 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmylk commented Oct 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmylk left a 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

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

Choose a reason for hiding this comment

markroxor Oct 17, 2016 • 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

tmylk commented Oct 17, 2016

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

markroxor commented Oct 18, 2016

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

tmylk commented Oct 20, 2016

markroxor commented Oct 20, 2016 • edited Loading

markroxor commented Oct 13, 2016 •

edited

Loading

markroxor Oct 14, 2016 •

edited

Loading

tmylk commented Oct 14, 2016 •

edited

Loading

markroxor Oct 17, 2016 •

edited

Loading

markroxor commented Oct 20, 2016 •

edited

Loading