-
Notifications
You must be signed in to change notification settings - Fork 615
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
Adding a tutorial on CyclicalLearningRate #2463
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Regarding code-styling, I have tried adhering to what's instructed here. |
@seanpmorgan do you mind taking a look when you get a moment? Many thanks. |
I think I have added the right links but the logs do not suggest so: Anything I might have missed? |
@sayakpaul hmmm I don't really see the issues with the links either. Can you try running this to pass the second test and perhaps something with spacing will fix the regex:
|
@seanpmorgan I actually ran it before creating the PR. That makes it even more surprising I guess. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@sayakpaul I was able to run the commands above and fix the format check, but I'm still unclear why those URLs are failing lint. @MarkDaoust when time allows do you mind taking a quick look if you're familiar with that test we're running? I think it's likely something trivial. |
Thank you, @seanpmorgan. Appreciate the help. |
@MarkDaoust a gentle reminder on this. If you could help us point in the right direction, that would be great. |
cc @lamberta. If time allows do you mind taking a quick look or letting us know if you've seen this before |
@seanpmorgan on a related note, apart from the build failure, is there anything I can do to improve/modify the tutorial notebook? |
Looks new to me. At least I haven't seen this on tf.org and don't find anything comparable on keras.io |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM.
Attached are the lint fix and a couple of minor suggestions.
"id": "MfBg1C5NB3X0" | ||
}, | ||
"source": [ | ||
"<table class=\"tfa-notebook-buttons\" align=\"left\">\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the problem. This needs to be tfo-notebook-buttons
(tfo == tensorflow.org). The checks and the site-css won't work otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Is there anything I can do to for progressing the PR? |
We have a linting failure. |
@bhack I ran the Docs formatter and pushed the new notebook. |
Thanks, @bhack. Approximately by when the tutorial will be live? Asking this because it's my first time. |
@bhack is there anything we could do to fix the build failure? |
This isn't automated. I'll run the update. |
I am sorry. First timer :( |
No worries Sayak! Thanks for submitting the tutorial. It's published: https://tensorflow.org/addons/tutorials/optimizers_cyclicallearningrate |
Description
Brief Description of the PR:
This PR adds a tutorial on
CyclicalLearningRate
.Fixes # (issue)
Type of change
Checklist:
How Has This Been Tested?
If you're adding a bugfix or new feature please describe the tests that you ran to verify your changes:
*