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

load() in 1.0.0 not rebuilding necessary down-sampling cum_table #1180

Closed
gojomo opened this issue Mar 3, 2017 · 5 comments
Closed

load() in 1.0.0 not rebuilding necessary down-sampling cum_table #1180

gojomo opened this issue Mar 3, 2017 · 5 comments

Comments

@gojomo
Copy link
Collaborator

gojomo commented Mar 3, 2017

See: https://groups.google.com/d/msg/gensim/F9SLBpik4Yg/Jz7BhRH_DQAJ

The test in Word2Vec load() is only chcking for index2word in its old location, thus a necessary structure for continued training isn't rebuilt.

Surprised no existing save/load tests catch this as the re-build should even be necessary for a save/load within the same latest gensim version.

@tmylk
Copy link
Contributor

tmylk commented Mar 3, 2017

Thanks. are you working on a fix or should I submit a pr?

@piskvorky
Copy link
Owner

@tmylk what is the explanation for this bug making it past the unit tests?

@gojomo
Copy link
Collaborator Author

gojomo commented Mar 3, 2017

Not currently working on this; just wanted to make sure there was an issue that could be triaged/assigned. Not sure the understanding above is fully correct - the code looks broken, but I would have expected any save/loadthat involves a (default) negative-sampling model to have triggered the error, so aspects of the problem as-reported are still unclear.

@gojomo
Copy link
Collaborator Author

gojomo commented Mar 3, 2017

To be clearer: any [save, load, train] should trigger the error - perhaps no existing tests try training after load? Adding either a train() or some other set of 'model is healthy' assertions after load() may make for better tests.

@tmylk
Copy link
Contributor

tmylk commented Mar 3, 2017

There is no unit test that covers training after load in either doc2vec or word2vec. Adding them now.

tmylk added a commit that referenced this issue Mar 3, 2017
@tmylk tmylk closed this as completed in 63cf941 Mar 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants