load_word_topics() returns value

[bhargavvader]

@tmylk, @piskvorky , with reference to #764 .

load_word_topics() now returns word topics which is assigned by train to self.wordtopics.

Before, there were two variables; word_topics and a wordtopics, and only the wordtopics variable was being used elsewhere; so I removed word_topics.

Is that ok?

[tmylk]

Rename wordtopics to word_topics.

Also should there be a meaningful error like 'run train or load_word_topics before showing topics' in https://github.com/bhargavvader/gensim/blob/6fd1ecbfe41d7ab2adc17d20179d467e986f477a/gensim/models/wrappers/ldamallet.py#L244

[bhargavvader]

Will do. Also, this PR doesn't seem to have a travis build happening, any reason?

[devashishd12]

oh renaming to word_topics would break some other things actually....

[tmylk]

@bhargavvader Well spotted about Travis - other PRs are working ok. Let's see what happens on your next commit.

[bhargavvader]

@dsquareindia , exactly what else would break? Not if we change all the wordtopics to word_topics right? The tests written still pass fine.

[devashishd12]

@bhargavvader the mallet2ldamodel would fail and also the support for ldamallet in topic coherence would fail. Tests pass because tests for one of them hasn't been added and for the other one hasn't been merged. I'll change the spelling in my PR but could you please change it in coherencemodel.py? Also a quick git grep would help.

[bhargavvader]

@dsquareindia , made the changes in test_ldamallet_wrapper.py, which is the only other place wordtopics shows up in.

coherencemodel.py doesn't seem to really use LdaMallet right now - but I do see it in PR #750 which you're working on, so it would make more sense for you to change it yourself.

[devashishd12]

yeah @bhargavvader I'll make both the changes in my pr once this gets merged. Thanks!

[bhargavvader]

@tmylk , @piskvorky , can you have a look?
Also, about the warning, I've just used logger.info now. Is that ok?

[tmylk]

warn instead of info

[bhargavvader]

Yup, did.

[tmylk]

Thanks for the PR. It's a useful fix!

[piskvorky]

-1 on renaming wordtopics to word_topics. This breaks backward compatibility for any code that uses that (including a lot of our own code). At least make it an alias.

[piskvorky]

This print makes no sense without the self.wordtopics assignment.

@tmylk did you review this before merging??

[bhargavvader]

You're right - the print was there in the code earlier and I removed the self.wordtopics assignment so now it doesn't make sense.

[bhargavvader]

@piskvorky , how exactly would one make an alias?
And what exactly do you mean by breaking the older code? I had checked for occurrences of wordtopics in the repo and couldn't find anything I didn't change. Or do you mean tutorials and the like?

I'll make another PR for that and for the unnecessary print statement. Is there anything else you think should change?

[piskvorky]

I mean, we use it in our own code, in commercial projects :)

I think it's safe to assume some other people depend on it too, in the same way. Unless it's unavoidable, we try to be backward-compatible with our changes.

Alias = two variables referring to the same object, i.e. x = y = some_var, or x = some_var; y = some_var.

[bhargavvader]

Ok, makes sense.

As for making an alias in this case, would you mean adding a line such as self.wordtopics = self.word_topics = word_topics or such whenever an assignment is being made?

[piskvorky]

Yes, assign self.wordtopics = self.word_topics, with a big fat comment explaining why this alias is there.

Option two: just continue to call the var self.wordtopics, like it was before.