-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Clear up job queue parameters in word2vec #2931
Conversation
The py36-win test didn't pass, so I put back the unused argument and only changed the names. Why is this argument only needed in the py36-win environment? |
If you look at the actual test failure in the logs, it was not related to your changes. It's a test with some randomness that's occasionally failed, so that test-run was unlucky, and the later succeeding run was lucky, neither affected by your changes. (I just added some changes that may make it less flaky in #2930 - if you re-base on current |
Looks like an improvement! But, having now looked at this more closely, the two different (current/update) methods seem redundant: they're both doing essentially the same thing. Want to take a try at going further, unifying them into one |
gensim/models/word2vec.py
Outdated
"""Get the correct learning rate for the next iteration. | ||
|
||
Parameters | ||
---------- | ||
job_params : dict of (str, obj) | ||
alpha : float |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is an unused parameter, and the method is internal, perhaps it would be safe to get rid of the parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will include a fix here as well.
@gojomo I see! I understand about the test failing. Also, thanks for the further specific suggestions for improvement! That's certainly true, so I'll fix this as well. |
Looks good to me! (I'll leave it to either @mpenkov or @piskvorky to do final approval/application.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Do any of the tutorials need updating after this API change? Did you / could you run a grep please?
Thanks for the confirmation! |
Awesome. Thanks :) |
Fixes: #2928
Small
job_queue
related changes.(list of object, (str, int))
(or(list of object, (str, float))
) to(list of object, float)
._get_job_params
and_update_job_params
are not properly named and have been replaced by_get_current_alpha
and_update_alpha
, respectively.job_params
have been replaced withalpha
._update_job_params
hadjob_params
passed to it, which has been removed because it wasn't being used.PS:
_get_current_alpha
and_update_alpha
do essentially the same thing, they have been merged into_get_next_alpha
.