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 cyclical learning rate schedulers #644

Merged
merged 10 commits into from
Nov 12, 2019
Merged

Add cyclical learning rate schedulers #644

merged 10 commits into from
Nov 12, 2019

Conversation

RaphaelMeudec
Copy link
Contributor

Migrating CyclicLR from keras-team/keras-contrib as discussed in keras-team/keras-contrib#519.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@RaphaelMeudec
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@gabrieldemarmiesse
Copy link
Member

I'm surprised. Do you guys have a continuous integration service running for pull requests?

Copy link
Member

@gabrieldemarmiesse gabrieldemarmiesse left a comment

Choose a reason for hiding this comment

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

Maybe some tests would help ensure that no bugs are introduced?

@RaphaelMeudec
Copy link
Contributor Author

I'm surprised. Do you guys have a continuous integration service running for pull requests?

Seems that CI is triggered by adding the kokoro:force-run tag

Maybe some tests would help ensure that no bugs are introduced?

Of course! Wanted to check first that implementation I used seems ok.

@gabrieldemarmiesse
Copy link
Member

Looks good to me :)

@seanpmorgan
Copy link
Member

Migrating CyclicLR from keras-team/keras-contrib as discussed in keras-team/keras-contrib#519.

Thanks @RaphaelMeudec implementation looks great. As far as test cases go, matching as many tests as you can from TF core schedulers is ideal.

We had a pretty nasty build break this morning... I will be looking into it shortly, but if you would like to run tests in the mean time just pin the nightly to yesterday's version:
https://github.com/tensorflow/addons/blob/master/build_deps/requirements.txt

If it doesn't seem there is a quick fix then we'll pin the master branch as well.

@RaphaelMeudec RaphaelMeudec changed the title [wip] Add cyclical learning rate schedulers Add cyclical learning rate schedulers Nov 4, 2019
@RaphaelMeudec
Copy link
Contributor Author

@seanpmorgan Added the tests, ready for review

@seanpmorgan
Copy link
Member

seanpmorgan commented Nov 5, 2019

Thanks @RaphaelMeudec! Looks like there are some line too long lint errors. Could you run this to auto format the code
bash tools/run_docker.sh -c 'make code-format'

@RaphaelMeudec
Copy link
Contributor Author

@seanpmorgan This is weird, the code-format is not doing any change. Is there a difference between the code-format and sanity-check? I'll fix the errors tomorrow

@WindQAQ
Copy link
Member

WindQAQ commented Nov 5, 2019

@seanpmorgan This is weird, the code-format is not doing any change. Is there a difference between the code-format and sanity-check? I'll fix the errors tomorrow

yapf does not aggressively make the length of lines fit 80 characters so you should manually modify it in most of the time.

@RaphaelMeudec
Copy link
Contributor Author

@WindQAQ Fixed the errors, should be good now

@RaphaelMeudec
Copy link
Contributor Author

@seanpmorgan Updated to register_keras_serializable, could you rerun kokoro?

@seanpmorgan seanpmorgan self-assigned this Nov 6, 2019
Copy link
Member

@seanpmorgan seanpmorgan left a comment

Choose a reason for hiding this comment

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

Implementation looks great. Small nit and then would you mind updating the TFA Optimizers README:
https://github.com/tensorflow/addons/blob/master/tensorflow_addons/optimizers/README.md

tensorflow_addons/optimizers/cyclical_learning_rate.py Outdated Show resolved Hide resolved
tensorflow_addons/optimizers/cyclical_learning_rate.py Outdated Show resolved Hide resolved
tensorflow_addons/optimizers/cyclical_learning_rate.py Outdated Show resolved Hide resolved
Copy link
Member

@seanpmorgan seanpmorgan left a comment

Choose a reason for hiding this comment

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

Thanks! One last nit

@@ -10,6 +10,7 @@
| moving_average | Dheeraj R. Reddy | dheeraj98reddy@gmail.com |
| rectified_adam | Zhao Hanguang | cyberzhg@gmail.com |
| weight_decay_optimizers | Phil Jund | ijund.phil@googlemail.com |
| cyclical_learning_rate | Raphael Meudec | raphael.meudec@gmail.com |
Copy link
Member

Choose a reason for hiding this comment

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

Alphabetize please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by alphabetize?

Copy link
Member

Choose a reason for hiding this comment

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

Just order alphabetically:

Copy link
Member

Choose a reason for hiding this comment

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

i.e. place it below conditional gradient in the list

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!

@@ -22,6 +23,7 @@
| moving_average | MovingAverage | |
| rectified_adam | RectifiedAdam | https://arxiv.org/pdf/1908.03265v1.pdf |
| weight_decay_optimizers | SGDW, AdamW, extend_with_decoupled_weight_decay | https://arxiv.org/pdf/1711.05101.pdf |
| cyclical_learning_rate | Cyclical Learning Rate | https://arxiv.org/abs/1506.01186 |
Copy link
Member

Choose a reason for hiding this comment

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

Alphabetize please

@RaphaelMeudec
Copy link
Contributor Author

@seanpmorgan Alphabetized it and rebased on tensorflow/addons. You might want to re-run kokoro

Copy link
Member

@seanpmorgan seanpmorgan left a comment

Choose a reason for hiding this comment

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

LGTM thanks for the contribution!

@seanpmorgan seanpmorgan merged commit 0a225c7 into tensorflow:master Nov 12, 2019
@gabrieldemarmiesse
Copy link
Member

Thanks a lot for the contribution @RaphaelMeudec and thank you @seanpmorgan for the review!

Copy link

@Hecim1984 Hecim1984 left a comment

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants