[Enhance] Support pass arugments with update_params #796
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
twmht
force-pushed
the
add_kwargs_to_optim_wrapper
branch
from
89d99e4 to
e6e4a6a
Compare
zhouzaida
previously approved these changes
Dec 7, 2022
HAOCHENYE
reviewed
Dec 7, 2022
| @@ -161,7 +161,7 @@ def __init__(self, | |||
| # the loss factor will always be the same as `_accumulative_counts`. | |||
| self._remainder_counts = -1 | |||
|
|
|||
| def update_params(self, loss: torch.Tensor) -> None: | |||
| def update_params(self, loss: torch.Tensor, **kwargs) -> None: | |||
There was a problem hiding this comment.
Suggested change
| def update_params(self, loss: torch.Tensor, **kwargs) -> None: | |
| def update_params(self, loss: torch.Tensor, step_kwargs=None, zero_grad=None) -> None: |
Consider update_params will call step and zero_grad in order, it could be better to provide step_kwargs and zero_kwargs arguments.
BTW, we should also update the description of the argument in docstring,
twmht
force-pushed
the
add_kwargs_to_optim_wrapper
branch
6 times, most recently
from
63267a7 to
97e2c43
Compare
twmht
force-pushed
the
add_kwargs_to_optim_wrapper
branch
from
97e2c43 to
9ad7e87
Compare
zhouzaida
reviewed
Dec 8, 2022
Comment on lines
172
to
173
| step_kwargs (dict): arguments for optimizer.step | ||
| zero_kwargs (dict): arguments for optimizer.zero_grad |
There was a problem hiding this comment.
Suggested change
| step_kwargs (dict): arguments for optimizer.step | |
| zero_kwargs (dict): arguments for optimizer.zero_grad | |
| step_kwargs (dict, optional): Arguments for optimizer.step. | |
| Defaults to None. | |
| New in version v0.4.0. | |
| zero_kwargs (dict, optional): Arguments for optimizer.zero_grad. | |
| Defaults to None. | |
| New in version v0.4.0. |
HAOCHENYE
reviewed
Dec 8, 2022
zhouzaida
reviewed
Dec 8, 2022
zhouzaida
approved these changes
Dec 8, 2022
|
I have Updated the code as you suggested. thank you |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
optimizer.step()supports kwargs (https://github.com/open-mmlab/mmengine/blob/main/mmengine/optim/optimizer/optimizer_wrapper.py#L211), butupdate_paramsnot (https://github.com/open-mmlab/mmengine/blob/main/mmengine/optim/optimizer/optimizer_wrapper.py#L176).this would be a problem if I want to write my own model wrapper.
Modification
Add kwargs to update_params
BC-breaking (Optional)
Does the modification introduce changes that break the backward-compatibility of the downstream repos?
If so, please describe how it breaks the compatibility and how the downstream projects should modify their code to keep compatibility with this PR.
Use cases (Optional)
If this PR introduces a new feature, it is better to list some use cases here, and update the documentation.
Checklist