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

add layerwise learning rate for adamw #35569

Merged
merged 6 commits into from
Sep 14, 2021

Conversation

zhaoyinglia
Copy link
Contributor

PR types

New features

PR changes

OPs

Describe

add layerwise learningrate feature for adamw

@paddle-bot-old
Copy link

paddle-bot-old bot commented Sep 8, 2021

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@zhaoyinglia zhaoyinglia changed the title Adamw layerwise add layerwise learning rate for adamw Sep 8, 2021
@@ -236,6 +236,10 @@ class AdamWOpMaker : public AdamOpMaker {
public:
void Make() {
AdamOpMaker::Make();
AddAttr<float>("lr_ratio",
Copy link
Contributor

Choose a reason for hiding this comment

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

why add this argument to adam? is that adamw and adam share the same .cc file ?

in this case, adamw should have its own .cc file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AdamWOpMaker inherits AdamOpMaker, and they use the same InferShape function of AdamOp.
In this case, 'lr_ratio' has no effect on Adam.

@@ -163,6 +165,9 @@ def __init__(self,
self._apply_decay_param_fun = apply_decay_param_fun
self._coeff = coeff
self._lr_to_coeff = dict()
if lr_ratio is not None:
assert isinstance(lr_ratio, Callable)
self._lr_ratio = lr_ratio
Copy link
Contributor

Choose a reason for hiding this comment

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

you should think about how many kernel will be affected by "lr_ratio".
if you only want the lr_ratio the affect gpu and cpu kernel, you should raise an Unimplement Error for xpu and npu here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@@ -163,6 +165,9 @@ def __init__(self,
self._apply_decay_param_fun = apply_decay_param_fun
self._coeff = coeff
self._lr_to_coeff = dict()
if lr_ratio is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

should add explanation for the new lr_ration argument, which should follow the explanation for "apply_decay_param_fun"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

PangHua
PangHua previously approved these changes Sep 14, 2021
Copy link
Contributor

@JZ-LIANG JZ-LIANG left a comment

Choose a reason for hiding this comment

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

LGTM

@JZ-LIANG JZ-LIANG merged commit 91cf918 into PaddlePaddle:develop Sep 14, 2021
AnnaTrainingG pushed a commit to AnnaTrainingG/Paddle that referenced this pull request Sep 29, 2021
* add layerwise learning rate for adamw

* fix format

* add unitest

* add NotImplementedError

* add gpu unitest

* update gpuplace
@sljlp sljlp mentioned this pull request Oct 16, 2021
@zhaoyinglia zhaoyinglia deleted the adamw_layerwise branch August 30, 2023 06:30
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.

3 participants