Skip to content

Conversation

@goldsborough
Copy link
Contributor

@ebetica asked for a way to add parameters to Optimizers after they are created.

@ebetica @ezyang

Copy link
Contributor

@ebetica ebetica left a comment

Choose a reason for hiding this comment

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

Note: one minor issue is that if any of the optimizers does significant work in its constructor, this might not work. Thus, add_parameters should definitely be virtual (and one of them should call the other so we don't have to implement two different add_parameter functions).

@goldsborough
Copy link
Contributor Author

I think making them virtual makes sense. To make one call the other, I'll have to update Cursor to have a method that yields a new vector with only the values, since currently a cursor is effectively a vector<pair<string, tensor>> which does't cooperate well with the tensor-only method. Should be a quick addition

@apaszke
Copy link
Contributor

apaszke commented Jul 17, 2018

FWIW this is not supported by the Python API (we do have add_param_group)

@ezyang
Copy link
Contributor

ezyang commented Jul 17, 2018

@apaszke Are you saying that the Python API should have this functionality, or that the C++ API shouldn't? ;)

@goldsborough
Copy link
Contributor Author

I didn't implement the whole parameter grouping functionality to begin with. It wasn't in autogradpp, and it seemed the better decision to start with only one parameter group than to have the added complexity of multiple parameter groups. Also, it can always be replicated with multiple optimizers right? TF doesn't have this parameter grouping behavior in their optimizers for example

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@goldsborough is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@apaszke
Copy link
Contributor

apaszke commented Jul 17, 2018

@ezyang I'm saying that they're inconsistent :)

Yeah, multiple optimizers give you the same results, they're just more annoying to maintain. I guess it's ok to leave it like that for now.

@ebetica
Copy link
Contributor

ebetica commented Jul 17, 2018

@apaszke I guess the core issue is that state_dict() and load_state_dict() are not present. Implement those would make me happy too. Just any way to load an optimizier's state.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@goldsborough has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

jramseyer pushed a commit to jramseyer/pytorch that referenced this pull request Jul 30, 2018
Summary:
ebetica asked for a way to add parameters to `Optimizer`s after they are created.

ebetica ezyang
Pull Request resolved: pytorch#9472

Differential Revision: D8872176

Pulled By: goldsborough

fbshipit-source-id: 39a4032c519a6d3b458dd3596361b04afea10365
goodlux pushed a commit to goodlux/pytorch that referenced this pull request Aug 15, 2018
Summary:
ebetica asked for a way to add parameters to `Optimizer`s after they are created.

ebetica ezyang
Pull Request resolved: pytorch#9472

Differential Revision: D8872176

Pulled By: goldsborough

fbshipit-source-id: 39a4032c519a6d3b458dd3596361b04afea10365
@ezyang ezyang added the merged label Jun 26, 2019
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.

5 participants