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.

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

Throw exception if load() is called on instance rather than the class #889

Merged
merged 42 commits into from
Dec 22, 2016
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
25fc004
Update word2vec.py
aayux Sep 26, 2016
58976d8
Update word2vec.py
aayux Sep 26, 2016
cf34e2d
Update word2vec.py
aayux Sep 26, 2016
773a75e
Update doc2vec.py
aayux Sep 26, 2016
96d65ec
Update doc2vec.py
aayux Sep 29, 2016
d91b99d
Update word2vec.py
aayux Sep 29, 2016
c156c79
Update utils.py
aayux Oct 24, 2016
60d42a3
Update word2vec.py
aayux Oct 24, 2016
cc7e3e1
Update utils.py
aayux Oct 24, 2016
a0ea2b4
Update word2vec.py
aayux Oct 24, 2016
2d85ef5
Update doc2vec.py
aayux Oct 24, 2016
ac0b0f3
Merge pull request #1 from dust0x/Issue-889
aayux Oct 24, 2016
770a277
Update doc2vec.py
aayux Oct 26, 2016
772cdcd
Update word2vec.py
aayux Oct 26, 2016
4c4ec8c
Update utils.py
aayux Oct 26, 2016
7173993
Update word2vec.py
aayux Oct 26, 2016
bb7b9e9
Update doc2vec.py
aayux Oct 26, 2016
23461e3
Merge branch 'develop' of https://github.com/RaRe-Technologies/gensim…
aayux Oct 26, 2016
fb5bd0b
Update doc2vec.py
aayux Oct 26, 2016
6ef3599
Update utils.py
aayux Oct 31, 2016
075d925
modified: gensim/utils.py
aayux Oct 31, 2016
bb53ccf
modified: gensim/test/test_doc2vec.py
aayux Oct 31, 2016
e39b8d6
modified: gensim/test/test_word2vec.py
aayux Oct 31, 2016
cec0a1b
modified: gensim/test/test_doc2vec.py
aayux Oct 31, 2016
c09e4d8
modified: gensim/test/test_doc2vec.py
aayux Oct 31, 2016
ea89bd5
modified: gensim/test/test_doc2vec.py
aayux Oct 31, 2016
5d743f1
modified: gensim/test/test_doc2vec.py
aayux Oct 31, 2016
d045bd2
modified: gensim/test/test_doc2vec.py
aayux Oct 31, 2016
07b3700
Merge https://github.com/RaRe-Technologies/gensim into develop
aayux Dec 4, 2016
40f2a50
Update test_word2vec.py
aayux Dec 4, 2016
43c7b08
Update test_doc2vec.py
aayux Dec 4, 2016
b50d60a
Update test_doc2vec.py
aayux Dec 4, 2016
eba33bc
Update test_word2vec.py
aayux Dec 4, 2016
6ffacdf
Update doc2vec.py
aayux Dec 4, 2016
f0137b8
Update doc2vec.py
aayux Dec 4, 2016
3bf25fc
Update test_word2vec.py
aayux Dec 4, 2016
7519519
Update test_word2vec.py
aayux Dec 4, 2016
2d28b03
Update test_word2vec.py
aayux Dec 4, 2016
19d2035
Update test_doc2vec.py
aayux Dec 4, 2016
e68bb13
Update test_word2vec.py
aayux Dec 4, 2016
37c9149
Merge pull request #2 from dust0x/tests
aayux Dec 4, 2016
d9b45c3
Merge branch 'develop' into develop
tmylk Dec 22, 2016
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
6 changes: 6 additions & 0 deletions gensim/models/doc2vec.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@
from six.moves import xrange, zip
from six import string_types, integer_types, itervalues

from types import MethodType

logger = logging.getLogger(__name__)

try:
Expand Down Expand Up @@ -608,6 +610,10 @@ def __init__(self, documents=None, size=300, alpha=0.025, window=8, min_count=5,
self.dbow_words = dbow_words
self.dm_concat = dm_concat
self.dm_tag_count = dm_tag_count

self.load = methodize(load, self)
self.load_word2vec_format = methodize(load, self)

if self.dm and self.dm_concat:
self.layer1_size = (self.dm_tag_count + (2 * self.window)) * self.vector_size
else:
Expand Down
34 changes: 34 additions & 0 deletions gensim/models/word2vec.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@
from six import iteritems, itervalues, string_types
from six.moves import xrange
from types import GeneratorType
from types import MethodType

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -452,6 +453,9 @@ def __init__(
self.total_train_time = 0
self.sorted_vocab = sorted_vocab
self.batch_words = batch_words

self.load = methodize(load, self)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean self.load = methodize(self.load, self) ?
it is very confusing to assign self.load and then define def load

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was done on purpose. Please see here for reference.

MethodType binds the method to the calling instance. What I'm doing here is something like overloading load based on whether it is called on instance or class.

So the argument load to methodize is the name of the function that is to be bound.

self.load_word2vec_format = methodize(load, self)

if sentences is not None:
if isinstance(sentences, GeneratorType):
Expand Down Expand Up @@ -1869,3 +1873,33 @@ def __iter__(self):
model.accuracy(args.accuracy)

logger.info("finished running %s", program)

def methodize(func, instance):
Copy link
Contributor

Choose a reason for hiding this comment

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

These methods are defined outside of the class. Maybe that is why you see no difference in calling these methods and class methods? Can you link to a gist of the code you use to test the first parameter?

Copy link
Contributor Author

@aayux aayux Sep 30, 2016

Choose a reason for hiding this comment

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

methods are defined outside of the class

This was done on purpose. Please see here for reference.

MethodType binds the method to the calling instance. What I'm doing here is something like overloading load based on whether it is called on instance or class.

So the argument load to methodize is the name of the function that is to be bound.

Copy link
Contributor Author

@aayux aayux Sep 30, 2016

Choose a reason for hiding this comment

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

My latest build is failing on Python 2 for some reason, I'm working on that right now. Once that's done, I'll write the tests and merge it back with develop.

return MethodType(func, instance, instance.__class__)


def load(self, *args, **kwargs):
logger.warn('Load was called on instance. Calling on class instead')
Word2Vec.load(*args, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not expected behaviour.
Expected behaviour is to print warning but still run the instance method.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest that best would be to error – this method doesn't modify an instance in-place, so the "one and only one right way to do it" is to call it is via the class.



def load_word2vec_format(
self,
fname,
fvocab=None,
binary=False,
encoding='utf8',
unicode_errors='strict',
limit=None,
datatype=REAL,
):
logger.warn('Load was called on instance. Calling on class instead')
Word2Vec.load_word2vec_format(
fname,
fvocab=None,
binary=False,
encoding='utf8',
unicode_errors='strict',
limit=None,
datatype=REAL,
)