Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Set idx2name for Optimizer object #14703

Merged
merged 2 commits into from
Apr 19, 2019
Merged

Set idx2name for Optimizer object #14703

merged 2 commits into from
Apr 19, 2019

Conversation

yuxihu
Copy link
Member

@yuxihu yuxihu commented Apr 15, 2019

Currently when we use module.fit(), we can pass a str or an Optimizer object as the optimizer. When we pass an Optimizer object that is created outside before calling module.fit(), we are not checking and setting optimizer.idx2name which is used to mask out entries in update for weight decay in module.init_optimizer method. If idx2name is not set when creating the optimizer object outside, it can impact the convergence behavior as weight decay is applied to all parameters in this case.

This PR checks if idx2name is set if an Optimizer object is given. If not, we are setting it to the proper values. Fixes #9511 as @wkcn pointed out.

@yuxihu yuxihu requested a review from szha as a code owner April 15, 2019 19:16
@yuxihu
Copy link
Member Author

yuxihu commented Apr 15, 2019

@ptrendx @romerojosh @apeforest @eric-haibin-lin Please help review.

@mxnet-label-bot add [pr-awaiting-review]

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label Apr 15, 2019
@wkcn
Copy link
Member

wkcn commented Apr 15, 2019

Great! It solves the issue #9511.

Copy link
Member

@wkcn wkcn left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

Copy link
Member

@eric-haibin-lin eric-haibin-lin left a comment

Choose a reason for hiding this comment

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

Any unit test?

@yuxihu
Copy link
Member Author

yuxihu commented Apr 16, 2019

Any unit test?

Not sure if there is any. If not, I can add one. Let me check.

@yuxihu
Copy link
Member Author

yuxihu commented Apr 17, 2019

@eric-haibin-lin Added unit test. Can you help review and merge?

@yuxihu
Copy link
Member Author

yuxihu commented Apr 18, 2019

@mxnet-label-bot update [pr-awaiting-merge]

@marcoabreu marcoabreu added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-awaiting-review PR is waiting for code review labels Apr 18, 2019
@wkcn wkcn merged commit 391a1be into apache:master Apr 19, 2019
@wkcn
Copy link
Member

wkcn commented Apr 19, 2019

Merged. Thank you!

@yuxihu yuxihu deleted the opt_idx2name branch April 19, 2019 15:33
kedarbellare pushed a commit to kedarbellare/incubator-mxnet that referenced this pull request Apr 20, 2019
* set idx2name for optimizer

* add unit test
yuxihu added a commit to yuxihu/incubator-mxnet that referenced this pull request Apr 22, 2019
* set idx2name for optimizer

* add unit test
szha pushed a commit that referenced this pull request Apr 23, 2019
* set idx2name for optimizer

* add unit test
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
* set idx2name for optimizer

* add unit test
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-merge Review and CI is complete. Ready to Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

set_lr_mult() or set_wd_mult() is invalid if not setting param_idx2name for the optimizer
4 participants