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

Adding LARS to ResNet #6327

Merged
merged 5 commits into from
Mar 11, 2019
Merged

Adding LARS to ResNet #6327

merged 5 commits into from
Mar 11, 2019

Conversation

pkanwar23
Copy link
Collaborator

Adding LARS to ResNet

@pkanwar23 pkanwar23 requested review from karmel and a team as code owners March 8, 2019 19:27
@robieta robieta requested review from dubey and tfboyd and removed request for karmel March 8, 2019 19:35
Copy link
Contributor

@robieta robieta left a comment

Choose a reason for hiding this comment

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

I have a couple minor comments, but overall this looks good. Thanks!

official/resnet/resnet_run_loop.py Outdated Show resolved Hide resolved
official/resnet/resnet_run_loop.py Show resolved Hide resolved
for batch size). The learning rate is then decayed using a polynomial rate
decay schedule with power 2.0.

Args:
Copy link
Member

Choose a reason for hiding this comment

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

Where are these args?

Copy link
Collaborator Author

@pkanwar23 pkanwar23 Mar 8, 2019

Choose a reason for hiding this comment

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

fixed.

@@ -44,6 +44,19 @@

DATASET_NAME = 'ImageNet'

flags.DEFINE_bool('enable_lars',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move these into define_resnet_flags in resnet_run_loop.py?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@tfboyd tfboyd left a comment

Choose a reason for hiding this comment

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

When you are finished with all the comments make sure you fix the lint errors we cleaned them all up and do not want to add new ones. You can see the errors by clicking the link that will take you to sponge. The log is hard to read but it tells you lines,files, and reasons.

Copy link
Member

@tfboyd tfboyd left a comment

Choose a reason for hiding this comment

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

LGTM from me. Lint looks ok and the very exact settings per batch are ok for now.

returns the current learning rate
"""

# Learning rate schedule for LARS polynomial schedule
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is different from before. Shouldn't it be:

if flags.FLAGS.batch_size < 8192:
  plr = 5.0
  w_epochs = 5
elif flags.FLAGS.batch_size < 16384:
  plr = 10.0
  w_epochs = 5
if flags.FLAGS.batch_size < 32768:
  plr = 25.0
  w_epochs = 5
else:
  plr = 32.0
  w_epochs = 14

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

facepalm. fixed.

@robieta
Copy link
Contributor

robieta commented Mar 9, 2019

Thanks! Enjoy the big shiny green button.

@pkanwar23 pkanwar23 merged commit 0b0dc7f into master Mar 11, 2019
wlongxiang pushed a commit to wlongxiang/models that referenced this pull request Apr 24, 2019
* Adding LARS to ResNet

* Fixes for the LARS patch

* Fixes for the LARS patch

* more fixes

* 1 more fix
@saberkun saberkun deleted the resnet-upgrade branch February 17, 2020 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants