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

Clear up job queue parameters in word2vec #2931

Merged
merged 3 commits into from
Sep 8, 2020

Conversation

lunastera
Copy link
Contributor

@lunastera lunastera commented Sep 2, 2020

Fixes: #2928

Small job_queue related changes.

  • Changed type information from (list of object, (str, int)) (or (list of object, (str, float))) to (list of object, float).
  • The _get_job_params and _update_job_params are not properly named and have been replaced by _get_current_alpha and _update_alpha, respectively.
  • Names like job_params have been replaced with alpha .
  • The _update_job_params had job_params passed to it, which has been removed because it wasn't being used.

PS:

  • Since _get_current_alpha and _update_alpha do essentially the same thing, they have been merged into _get_next_alpha.

@lunastera
Copy link
Contributor Author

lunastera commented Sep 2, 2020

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?

@gojomo
Copy link
Collaborator

gojomo commented Sep 3, 2020

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 develop it should minimize further distracting unrelated errors.)

@gojomo
Copy link
Collaborator

gojomo commented Sep 3, 2020

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 _get_next_alpha() method, that (at the outset) just considers current-epoch-progress as 0.0, & maybe even has simpler but consistent calculation-logic?

"""Get the correct learning rate for the next iteration.

Parameters
----------
job_params : dict of (str, obj)
alpha : float
Copy link
Collaborator

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?

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 will include a fix here as well.

@lunastera
Copy link
Contributor Author

lunastera commented Sep 4, 2020

@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.

@gojomo
Copy link
Collaborator

gojomo commented Sep 6, 2020

Looks good to me! (I'll leave it to either @mpenkov or @piskvorky to do final approval/application.)

Copy link
Owner

@piskvorky piskvorky left a 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?

@lunastera
Copy link
Contributor Author

lunastera commented Sep 8, 2020

Thanks for the confirmation!
I did a grep on the changed variables and functions, but I didn't find any changes.

@piskvorky piskvorky changed the title Corrected info about elements of the job queue Clear up job queue parameters in word2vec Sep 8, 2020
@piskvorky
Copy link
Owner

Awesome. Thanks :)

@piskvorky piskvorky merged commit 9cd72f5 into piskvorky:develop Sep 8, 2020
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

Successfully merging this pull request may close these issues.

The type information about job_queue in word2vec is wrong
4 participants