Fix implementation of smartirs Document Frequency n

[PeteBleackley]

Description

tfidfmodel.updated_wglobal(docfreq, totaldoc, 'n') returns utils.identity(docfreq) instead of 1.
This means that all term frequencies will be multiplied by the document frequency. For a large corpus this is particularly bad and will cause normalisation to crash.

[markroxor]

Please take a look at the model again wglobal is actually used here

return {termid: wglobal(df, total_docs) for termid, df in iteritems(dfs)}

wglobal returns the normalized doc_freq it is not a normalizing factor therefore when tfidfmodel.updated_wglobal(docfreq, totaldoc, 'n') is used it should return docfreq

[markroxor]

@menshikh-iv please close this issue.

[PeteBleackley]

@markroxor According to https://en.m.wikipedia.org/wiki/SMART_Information_Retrieval_System , df="n" should return 1. Returning docfreq causes serious problems with a large corpus (*nc crashes due to a numerical overflow), and it was because of this that I raised the issue and the PR.

[markroxor]

I have just used the terminologies and not the exact approach. Please go through the code bit by bit and you will understand.

[PeteBleackley]

@markroxor I've just spent a week working out why my code kept crashing, and it turned out to be because the implementation of SMART doesn't match the published specification. I think you wanted "n" to mean "Do not modify the argument" in all positions, but that doesn't make sense for document frequencies, especially as you generally want smaller weights for larger DFs. It would make more sense to think of it as "Do not modify TF".

[markroxor]

I think you wanted "n" to mean "Do not modify the argument" in all positions,

You are right but can you see the code flow and where exactly the function updated_wglobal is used?
It is used here.

According to your modification updated_global will return 1, and therefore the precompute_idfs will return a dictionary with all values as 1. Please go through the code flow.

I've just spent a week working out why my code kept crashing, and it turned out to be because the implementation of SMART doesn't match the published specification.

The problem can possibly be with your code and not with tf-idf implementation. Feel free to share your code at the gensim gitter channel.

[PeteBleackley]

@markroxor Yes, precompute_idfs should return all ones if the df argument is n.

To reproduce the bug I encountered, use the following corpus class.

import nltk.corpus
import gensim

class BrownCorpus(gensim.corpora.TextCorpus):
    
    def __init__(self,input):
        self.corpus=nltk.corpus.brown
        super(BrownCorpus,self).__init__(input)
        
        
    def get_texts(self):
        for para in self.corpus.paras():
            result=[]
            for sentence in para:
                result.extend(sentence)
            yield result

Train a TfidfModel on that with smartirs="nnc". Then try to transform a document with it.

[markroxor]

Yes, precompute_idfs should return all ones if the df argument is n.

If that is what is intended than I agree with your concern. I am going to review your PR suggesting the necessary changes.