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

Enable optimizer model setup with instantiation #3488

Merged
merged 4 commits into from Nov 21, 2017

Conversation

Crissman
Copy link
Member

@Crissman Crissman commented Oct 4, 2017

Code to close issue #3211 .

Allow models to be passed as an argument to an optimizer, so

optimizer = chainer.optimizers.SGD()
optimizer.setup(model)

becomes:

optimizer = chainer.optimizers.SGD(link=model)

Change added to all optimizers. Old two-line method also still valid.

Tested by running new one-line method and old method on SGD, then running new method on all optimizers after changing code.

@mitmul
Copy link
Member

mitmul commented Oct 4, 2017

@Crissman Check the travis-ci's test errors and please fix them.

@niboshi niboshi added the cat:feature Implementation that introduces new interfaces. label Oct 4, 2017
@mitmul
Copy link
Member

mitmul commented Nov 21, 2017

Jenkins, test this please

@mitmul
Copy link
Member

mitmul commented Nov 21, 2017

LGTM

@mitmul mitmul merged commit 016cffa into chainer:master Nov 21, 2017
@mitmul mitmul added the backport Pull request that is backported from a more recent version. label Nov 21, 2017
@mitmul mitmul added this to the v4.0.0b2 milestone Nov 21, 2017
@mitmul mitmul added to-be-backported Pull request that should be backported. and removed backport Pull request that is backported from a more recent version. labels Nov 21, 2017
@niboshi
Copy link
Member

niboshi commented Nov 21, 2017

Note that this code does not work, because the first argument of SGD is lr.

optimizer = chainer.optimizers.SGD(model)

I think it's better to make model a keyword-only argument.

@Crissman
Copy link
Member Author

@niboshi Correct. model should be a keyword argument. I've updated my comment above accordingly.

@Crissman Crissman deleted the opt-setup branch November 22, 2017 04:52
@kmaehashi kmaehashi removed the to-be-backported Pull request that should be backported. label Dec 7, 2017
@kmaehashi
Copy link
Member

Removed to-be-backported (as discussed in #3936)

toslunar added a commit to toslunar/chainer that referenced this pull request Dec 22, 2017
This reverts commit 016cffa, reversing
changes made to a054cd6.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:feature Implementation that introduces new interfaces.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants