Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update sklearn API for Gensim models #1473

Merged
merged 27 commits into from
Aug 10, 2017
Merged

Update sklearn API for Gensim models #1473

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
c55fcc9
renamed sklearn wrapper classes
Jul 6, 2017
dde234f
added newline for flake8 check
Jul 7, 2017
721806b
renamed sklearn api files
Jul 10, 2017
3cdcfde
updated tests for sklearn api
Jul 10, 2017
99dba85
updated ipynb for sklearn api
Jul 10, 2017
4d1eaf4
PEP8 changes
Jul 10, 2017
155a1ec
updated docstrings for sklearn wrappers
Jul 11, 2017
ae6c0f3
added 'testPersistence' and 'testModelNotFitted' tests for author top…
Jul 11, 2017
3c78873
removed 'set_params' function from all wrappers
chinmayapancholi13 Jul 13, 2017
341ed1f
removed 'get_params' function from base class
chinmayapancholi13 Jul 14, 2017
9113f82
removed 'get_params' function from all api classes
chinmayapancholi13 Jul 14, 2017
2935680
removed 'partial_fit()' from base class
chinmayapancholi13 Jul 19, 2017
9628f99
updated error message
chinmayapancholi13 Jul 19, 2017
3849d06
updated error message for 'partial_fit' function in W2VTransformer
chinmayapancholi13 Jul 20, 2017
6097349
removed 'BaseTransformer' class
chinmayapancholi13 Jul 26, 2017
5b21875
updated error message for 'partial_fit' in 'W2VTransformer'
chinmayapancholi13 Jul 26, 2017
6bfdb4d
added checks for setting attributes after calling 'fit'
chinmayapancholi13 Jul 27, 2017
9f0be87
flake8 fix
chinmayapancholi13 Jul 27, 2017
6004eee
using 'sparse2full' in 'transform' function
chinmayapancholi13 Jul 27, 2017
3262ec2
added missing imports
chinmayapancholi13 Jul 27, 2017
d4e560e
added comment about returning dense representation in 'transform' fun…
chinmayapancholi13 Jul 27, 2017
ad3f1f7
added 'testConsistencyWithGensimModel' for ldamodel
chinmayapancholi13 Jul 27, 2017
877632e
updated ipynb
chinmayapancholi13 Jul 27, 2017
0871b50
updated 'testPartialFit' for Lda and Lsi transformers
chinmayapancholi13 Jul 28, 2017
3f363a1
added author info
chinmayapancholi13 Jul 28, 2017
c0894bc
added 'testConsistencyWithGensimModel' for w2v transformer
chinmayapancholi13 Jul 28, 2017
9b7402d
removed merge conflicts
chinmayapancholi13 Aug 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
562 changes: 562 additions & 0 deletions docs/notebooks/sklearn_api.ipynb
View file
Open in desktop

Large diffs are not rendered by default.

683 changes: 0 additions & 683 deletions docs/notebooks/sklearn_wrapper.ipynb
View file
Open in desktop

This file was deleted.

19 changes: 19 additions & 0 deletions gensim/sklearn_api/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#!/usr/bin/env python
# -*- coding: utf-8 -*-
#
# Copyright (C) 2011 Radim Rehurek <[email protected]>
Copy link
Owner

Choose a reason for hiding this comment

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

@chinmayapancholi13 please add yourself as the author. This is your baby :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hehe. Ok! 😄

# Licensed under the GNU LGPL v2.1 - http://www.gnu.org/licenses/lgpl.html
"""Scikit learn wrapper for gensim.
Contains various gensim based implementations which match with scikit-learn standards.
See [1] for complete set of conventions.
[1] http://scikit-learn.org/stable/developers/
"""


from .basemodel import BaseTransformer # noqa: F401
from .ldamodel import LdaTransformer # noqa: F401
Copy link
Owner

Choose a reason for hiding this comment

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

What is all that noqa: F401 for? Is it really necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A line having noqa doesn't issue flake8 errors. Writing noqa: F401 saves us from flake8 errors saying that we have imports (e.g. LdaTransformer) which we haven't used anywhere in the file.

Copy link
Owner

Choose a reason for hiding this comment

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

It feels wrong to litter the gensim code with such constructs -- the gensim code is correct, this is essentially working around some idiosyncrasy (bug?) of an unrelated library.

By the way, how come we don't these errors from all the other __init__ imports in gensim? Or do we? CC @menshikh-iv

Copy link
Contributor

Choose a reason for hiding this comment

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

@piskvorky because flake8 analyze the only diff every time, if you change same lines in any other __init__ file you will get same error.

Copy link
Owner

@piskvorky piskvorky Jul 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

OK, so the fake warnings will go away immediately? Why did we add these comments then. Let's remove it.

from .lsimodel import LsiTransformer # noqa: F401
from .rpmodel import RpTransformer # noqa: F401
from .ldaseqmodel import LdaSeqTransformer # noqa: F401
from .w2vmodel import W2VTransformer # noqa: F401
from .atmodel import AuthorTopicTransformer # noqa: F401
6 changes: 3 additions & 3 deletions ...gration/sklearn_wrapper_gensim_atmodel.py → gensim/sklearn_api/atmodel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@
from sklearn.exceptions import NotFittedError

from gensim import models
from gensim.sklearn_integration import BaseSklearnWrapper
from gensim.sklearn_api import BaseTransformer


class SklATModel(BaseSklearnWrapper, TransformerMixin, BaseEstimator):
class AuthorTopicTransformer(BaseTransformer, TransformerMixin, BaseEstimator):
"""
Base AuthorTopic module
"""
Expand Down Expand Up @@ -65,7 +65,7 @@ def set_params(self, **parameters):
"""
Set all parameters.
"""
super(SklATModel, self).set_params(**parameters)
super(AuthorTopicTransformer, self).set_params(**parameters)
return self

def fit(self, X, y=None):
Expand Down
2 changes: 1 addition & 1 deletion ...learn_integration/base_sklearn_wrapper.py → gensim/sklearn_api/basemodel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
from abc import ABCMeta, abstractmethod


class BaseSklearnWrapper(object):
class BaseTransformer(object):
Copy link
Collaborator

Choose a reason for hiding this comment

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

With this refactoring... I'm again wondering, is this class even helpful? Its set_params() does less than the implementation that would be inherited from sklearn's own BaseEstimator. The other methods just serve to enforce that any subclass override certain methods – a rigor that is possibly useful, but that sklearn does not, itself, impose within its own superclass-hierarchy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gojomo scikit-learn indeed does not impose such a hierarchy among its classes. However, as you stated above, in addition to not having to implement the set_params() every time, we know which functions are to be implemented for each model. Also, if in the future, we decide to refine/improve the set_params() function and make it more sophisticated (e.g. as in sklearn's BaseEstimator), we would only have to do it once in the base class rather than having to make the same change in the sklearn-api class of each Gensim model. Hence, having an abstract base class and deriving classes for Gensim models from it would help us in writing new sklearn-api classes in the future as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But, the existing set_params() looks worse than just doing nothing (and inheriting from BaseEstimator as all current subclasses of this class already do) – unless I'm missing something. So there's no need to plan for a future where "we decide to refine/improve" it, and do such improvements in the base class. We can make that decision now, and delete 8 lines of code from the base class, and all the subclasses automatically get the better & more standard (for sklearn) implementation of set_params().

And then, the only benefit of this shared superclass is forcing an error as a reminder of what to implement. But if sklearn doesn't do that itself, is it that important to do so? It's also over-enforcing – it is not a strict requirement that all transformers support partial_fit() - not every algorithm will necessarily support that.

Copy link
Contributor

@tmylk tmylk Jul 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

In my head I expect all sklearn models to have partial_fit, fit, transform and be used in a pipeline. We constantly get questions on the mailing list about how to update model X for almost every sklearn algo. Raising a NotImplemented with an explanation for word2vec and doc2vec use gensim api update function but it's not recommended as it hasn't been researched. And throw an explicitly that RandomProj can't do it because of algo limitations.

Also set_params method shouldn't be abstract and shouldn't be defined in all the children -it's the same code. Having common set_params makes the base useful.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, providing a place to include a error/warning is a potential reason to implement partial_fit() even where the underlying algorithm can't (or shouldn't casually) be used that way. BUT, simply not implementing a method is also a pretty good way of signalling that something isn't implemented – and indeed that seems to be what scikit-learn itself does. For that reason, it's even possible other code will use introspection about whether an sklearn pipeline object implements partial_fit() as a test of support, in which case having an implemented-method that just errors could be suboptimal.

But with set_params(), again, why have any lesser-featured implementation here, when every subclass (so far) inherits from scikit-learn's own BaseEstimator, which provides a more fully-featured implementation? Why wouldn't we want fewer limitations (and match the source library's behavior) for zero lines of code, rather than more limitations with more code?

Copy link

Choose a reason for hiding this comment

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

hi,

jumping in the conversation a little late here, but I do agree that the old BaseSklearnWrapper does very little since

  • the default get_params and set_params from sklearn's BaseEstimator are very reasonable and should only be overriden when necessary; if all the concrete wrapper classes inherit from BaseEstimator (as they do now) then there's no need to force them to re-implement these methods
  • @tmylk partial_fit is absolutely not a requirement for a class to be compatible with sklearn Pipeline since lots of algorithm are not online in nature. If there is a particular algorithm that most people are confused about, then the warning should be raised for that class only.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would struggle to find a gensim algorithm for which I haven't seen a "is it possible to update the model post-training?" question on the mailing list...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, it's always asked, but not always supported. The scikit-learn convention for not supporting incremental training is to simply not implement partial_fit(). Why not adopt the same convention for scikit-learn compatible classes here?

Copy link
Owner

@piskvorky piskvorky Jul 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with @gojomo 's point of view here. The point of these adapters is to assume the "philosophy" of another package, whatever structure or idiosyncrasies that may have. The least amount of surprises and novelty for users of that package.

It is not the place of the adapters to introduce their own philosophy on class structures or missing methods or whatever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed the BaseTransformer class now. Thanks for the useful feedback and suggestions. :)

"""
Base sklearn wrapper module
"""
Expand Down
12 changes: 6 additions & 6 deletions ...ration/sklearn_wrapper_gensim_ldamodel.py → gensim/sklearn_api/ldamodel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@

from gensim import models
from gensim import matutils
from gensim.sklearn_integration import BaseSklearnWrapper
from gensim.sklearn_api import BaseTransformer


class SklLdaModel(BaseSklearnWrapper, TransformerMixin, BaseEstimator):
class LdaTransformer(BaseTransformer, TransformerMixin, BaseEstimator):
"""
Base LDA module
"""
Expand Down Expand Up @@ -63,7 +63,7 @@ def set_params(self, **parameters):
"""
Set all parameters.
"""
super(SklLdaModel, self).set_params(**parameters)
super(LdaTransformer, self).set_params(**parameters)
return self

def fit(self, X, y=None):
Expand All @@ -86,8 +86,8 @@ def fit(self, X, y=None):

def transform(self, docs):
"""
Takes as an list of input a documents (documents).
Returns matrix of topic distribution for the given document bow, where a_ij
Takes a list of documents as input ('docs').
Returns a matrix of topic distribution for the given document bow, where a_ij
indicates (topic_i, topic_probability_j).
The input `docs` should be in BOW format and can be a list of documents like : [ [(4, 1), (7, 1)], [(9, 1), (13, 1)], [(2, 1), (6, 1)] ]
or a single document like : [(4, 1), (7, 1)]
Expand All @@ -105,7 +105,7 @@ def transform(self, docs):
probs_docs = list(map(lambda x: x[1], doc_topics))
# Everything should be equal in length
if len(probs_docs) != self.num_topics:
probs_docs.extend([1e-12]*(self.num_topics - len(probs_docs)))
probs_docs.extend([1e-12] * (self.num_topics - len(probs_docs)))
X[k] = probs_docs
return np.reshape(np.array(X), (len(docs), self.num_topics))

Expand Down
8 changes: 4 additions & 4 deletions ...ion/sklearn_wrapper_gensim_ldaseqmodel.py → gensim/sklearn_api/ldaseqmodel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@
from sklearn.exceptions import NotFittedError

from gensim import models
from gensim.sklearn_integration import BaseSklearnWrapper
from gensim.sklearn_api import BaseTransformer


class SklLdaSeqModel(BaseSklearnWrapper, TransformerMixin, BaseEstimator):
class LdaSeqTransformer(BaseTransformer, TransformerMixin, BaseEstimator):
"""
Base LdaSeq module
"""
Expand Down Expand Up @@ -60,7 +60,7 @@ def set_params(self, **parameters):
"""
Set all parameters.
"""
super(SklLdaSeqModel, self).set_params(**parameters)
super(LdaSeqTransformer, self).set_params(**parameters)
return self

def fit(self, X, y=None):
Expand Down Expand Up @@ -99,4 +99,4 @@ def transform(self, docs):
return np.reshape(np.array(X), (len(docs), self.num_topics))

def partial_fit(self, X):
raise NotImplementedError("'partial_fit' has not been implemented for SklLdaSeqModel")
raise NotImplementedError("'partial_fit' has not been implemented for LdaSeqTransformer")
12 changes: 6 additions & 6 deletions ...ration/sklearn_wrapper_gensim_lsimodel.py → gensim/sklearn_api/lsimodel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@

from gensim import models
from gensim import matutils
from gensim.sklearn_integration import BaseSklearnWrapper
from gensim.sklearn_api import BaseTransformer


class SklLsiModel(BaseSklearnWrapper, TransformerMixin, BaseEstimator):
class LsiTransformer(BaseTransformer, TransformerMixin, BaseEstimator):
"""
Base LSI module
"""
Expand Down Expand Up @@ -50,7 +50,7 @@ def set_params(self, **parameters):
"""
Set all parameters.
"""
super(SklLsiModel, self).set_params(**parameters)
super(LsiTransformer, self).set_params(**parameters)
return self

def fit(self, X, y=None):
Expand Down Expand Up @@ -81,13 +81,13 @@ def transform(self, docs):
# The input as array of array
check = lambda x: [x] if isinstance(x[0], tuple) else x
docs = check(docs)
X = [[] for i in range(0,len(docs))];
for k,v in enumerate(docs):
X = [[] for i in range(0, len(docs))]
for k, v in enumerate(docs):
doc_topics = self.gensim_model[v]
probs_docs = list(map(lambda x: x[1], doc_topics))
# Everything should be equal in length
if len(probs_docs) != self.num_topics:
probs_docs.extend([1e-12]*(self.num_topics - len(probs_docs)))
probs_docs.extend([1e-12] * (self.num_topics - len(probs_docs)))
X[k] = probs_docs
probs_docs = []
return np.reshape(np.array(X), (len(docs), self.num_topics))
Expand Down
8 changes: 4 additions & 4 deletions ...gration/sklearn_wrapper_gensim_rpmodel.py → gensim/sklearn_api/rpmodel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@
from sklearn.exceptions import NotFittedError

from gensim import models
from gensim.sklearn_integration import BaseSklearnWrapper
from gensim.sklearn_api import BaseTransformer


class SklRpModel(BaseSklearnWrapper, TransformerMixin, BaseEstimator):
class RpTransformer(BaseTransformer, TransformerMixin, BaseEstimator):
"""
Base RP module
"""
Expand All @@ -40,7 +40,7 @@ def set_params(self, **parameters):
"""
Set all parameters.
"""
super(SklRpModel, self).set_params(**parameters)
super(RpTransformer, self).set_params(**parameters)
return self

def fit(self, X, y=None):
Expand Down Expand Up @@ -77,4 +77,4 @@ def transform(self, docs):
return np.reshape(np.array(X), (len(docs), self.num_topics))

def partial_fit(self, X):
raise NotImplementedError("'partial_fit' has not been implemented for SklRpModel")
raise NotImplementedError("'partial_fit' has not been implemented for RpTransformer")
8 changes: 4 additions & 4 deletions ...ration/sklearn_wrapper_gensim_w2vmodel.py → gensim/sklearn_api/w2vmodel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@
from sklearn.exceptions import NotFittedError

from gensim import models
from gensim.sklearn_integration import BaseSklearnWrapper
from gensim.sklearn_api import BaseTransformer


class SklW2VModel(BaseSklearnWrapper, TransformerMixin, BaseEstimator):
class W2VTransformer(BaseTransformer, TransformerMixin, BaseEstimator):
"""
Base Word2Vec module
"""
Expand Down Expand Up @@ -66,7 +66,7 @@ def set_params(self, **parameters):
"""
Set all parameters.
"""
super(SklW2VModel, self).set_params(**parameters)
super(W2VTransformer, self).set_params(**parameters)
return self

def fit(self, X, y=None):
Expand Down Expand Up @@ -101,4 +101,4 @@ def transform(self, words):
return np.reshape(np.array(X), (len(words), self.size))

def partial_fit(self, X):
raise NotImplementedError("'partial_fit' has not been implemented for SklW2VModel")
raise NotImplementedError("'partial_fit' has not been implemented for W2VTransformer")
19 changes: 0 additions & 19 deletions gensim/sklearn_integration/__init__.py
View file
Open in desktop

This file was deleted.

Loading