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.

Sign up for GitHub

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

Jump to bottom

Make updaters call new_epoch automatically #4608

Merged
merged 4 commits into from Jun 18, 2018
Merged

Make updaters call new_epoch automatically #4608

merged 4 commits into from Jun 18, 2018

Conversation

beam2d
Copy link
Member

@beam2d beam2d commented Apr 11, 2018

Fixed #3044. This PR lets the built-in updaters automatically call Optimizer.new_epoch() when is_new_epoch is true for the main iterator.

This change is incompatible with v4. To aid the incompatibility, I added use_auto_new_epoch flag to Optimizer, which is set by the updaters. When this flag is set, new_epoch() called outside the updater raises an error so that users can notice duplicated calls of new_epoch(). It means that existing code that uses any updater and new_epoch simultaneously will raise an error. This error can be fixed by 1) not calling new_epoch outside the updater, or 2) passing auto_new_epoch=False to the updater.

@beam2d beam2d added cat:feature Implementation that introduces new interfaces. no-compat Change that disrupts backward compatibility. labels Apr 11, 2018
@niboshi niboshi self-assigned this May 28, 2018
@niboshi niboshi added the st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. label Jun 5, 2018
@niboshi niboshi added this to the v5.0.0b2 milestone Jun 5, 2018
@niboshi
Copy link
Member

niboshi commented Jun 7, 2018

Please fix the style error.

@niboshi niboshi added st:awaiting-author State indicating that response is needed from contributors, often authors of pull request. and removed st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. labels Jun 7, 2018
@beam2d
Copy link
Member Author

beam2d commented Jun 7, 2018

Fixed

@niboshi niboshi added st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. and removed st:awaiting-author State indicating that response is needed from contributors, often authors of pull request. labels Jun 7, 2018
@niboshi
Copy link
Member

niboshi commented Jun 7, 2018

jenkins, test this please

@niboshi
Copy link
Member

niboshi commented Jun 8, 2018

Please check test failure.

@niboshi niboshi removed the st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. label Jun 8, 2018
@niboshi niboshi added the st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. label Jun 12, 2018
@niboshi
Copy link
Member

niboshi commented Jun 13, 2018

chainer/optimizer.py:docstring of chainer.Optimizer:None: WARNING: more than one target found for cross-reference 'use_auto_new_epoch': chainer.optimizers.MomentumSGD.use_auto_new_epoch, chainer.optimizers.SGD.use_auto_new_epoch, chainer.optimizers.AdaGrad.use_auto_new_epoch, chainer.optimizers.RMSprop.use_auto_new_epoch, chainer.GradientMethod.use_auto_new_epoch, chainer.optimizers.SMORMS3.use_auto_new_epoch, chainer.Optimizer.use_auto_new_epoch, chainer.optimizers.NesterovAG.use_auto_new_epoch, chainer.optimizers.Adam.use_auto_new_epoch, chainer.optimizers.AdaDelta.use_auto_new_epoch, chainer.optimizers.RMSpropGraves.use_auto_new_epoch

It seems you need to specify class names in the docstrings.

@niboshi niboshi added st:awaiting-author State indicating that response is needed from contributors, often authors of pull request. and removed st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. labels Jun 13, 2018
@beam2d
Copy link
Member Author

beam2d commented Jun 18, 2018

Fixed the docstring.

@niboshi niboshi added st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. and removed st:awaiting-author State indicating that response is needed from contributors, often authors of pull request. labels Jun 18, 2018
@chainer-ci
Copy link
Member

Jenkins CI test (for commit 47bd02e) failed with status FAILURE.
(For contributors, please wait until the reviewer confirms the details of the error.)

@niboshi
Copy link
Member

niboshi commented Jun 18, 2018

jenkins, test this please

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 47bd02e) failed with status FAILURE.
(For contributors, please wait until the reviewer confirms the details of the error.)

@niboshi
Copy link
Member

niboshi commented Jun 18, 2018

LGTM!

@niboshi niboshi changed the title Make updaters call new_epoch automatically Make updaters call new_epoch automatically Jun 18, 2018
@niboshi niboshi merged commit f19ef9d into chainer:master Jun 18, 2018
kmaehashi added a commit to kmaehashi/chainer that referenced this pull request Jun 19, 2018
@kmaehashi
add upgrade guide for no-compat changes in chainer#4608
35a7bb2
@kmaehashi kmaehashi mentioned this pull request Jun 19, 2018
Merged
dBeker pushed a commit to dBeker/chainer that referenced this pull request Sep 10, 2018
@kmaehashi
add upgrade guide for no-compat changes in chainer#4608
4c3ea0e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@niboshi niboshi

Labels
cat:feature Implementation that introduces new interfaces. no-compat Change that disrupts backward compatibility. st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes.
Projects
None yet
Milestone
v5.0.0b2
Development

Successfully merging this pull request may close these issues.

StandardUpdater should call Optimizer.new_epoch appropriately
3 participants
@beam2d @niboshi @chainer-ci