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

Fix the serialization bug of rectified adam. #1375

Merged
merged 3 commits into from
Mar 24, 2020
Merged

Fix the serialization bug of rectified adam. #1375

merged 3 commits into from
Mar 24, 2020

Conversation

gabrieldemarmiesse
Copy link
Member

@gabrieldemarmiesse gabrieldemarmiesse commented Mar 24, 2020

Fix #1373

@bot-of-gabrieldemarmiesse

@CyberZHG

You are owner of some files modified in this pull request.
Would you kindly review the changes whenever you have the time to?
Thank you very much.

@gabrieldemarmiesse gabrieldemarmiesse added the backport r0.8 Will backport any PR merged with this label to the r0.8 branch label Mar 24, 2020
Copy link
Member

@Squadrick Squadrick left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. It's unfortunate this was overlooked during the reviews.

tensorflow_addons/optimizers/rectified_adam.py Outdated Show resolved Hide resolved
if isinstance(total_steps, float):
warnings.warn(
"The parameter `total_steps` passed to the __init__ of RectifiedAdam "
"is a float. This behavior is deprecated and in Addons 0.11, this "
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to select 0.11? Also, how do we keep track of this when we get to version 0.11?

Copy link
Member Author

@gabrieldemarmiesse gabrieldemarmiesse Mar 24, 2020

Choose a reason for hiding this comment

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

We need to give a date in the warning, otherwise users won't care. They'll just say: "I'll leave tomorrow's probems to tomorrow's me". :P

No idea how to track it though. Milestones in github?

Copy link
Member

Choose a reason for hiding this comment

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

Milestones require a due date, not version.

Why not provide a deprecation date instead of a version? But this directly means we'll need to create a new release on that due date.

Copy link
Member Author

@gabrieldemarmiesse gabrieldemarmiesse Mar 24, 2020

Choose a reason for hiding this comment

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

When stopping support for a branch, it's indeed common to specify a date: ex the python org will stop support for the branch 2.7 of python january 1st 2020.

When deprecating a feature and removing it later, it's more common to use a specific version:
/opt/conda/lib/python3.7/site-packages/tensorflow/python/training/tracking/data_structures.py:718: DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated since Python 3.3,and in 3.9 it will stop working.

I believe it makes more sense to use a specific version. We can make a milestone. We're not forced to add a date. For example:
https://github.com/pytest-dev/pytest/milestones

Copy link
Member

Choose a reason for hiding this comment

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

Sounds great! I'll accept this PR. We should create a milestone for this as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I can do that.

Copy link
Member

Choose a reason for hiding this comment

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

Haha, accidentally created a duplicate issue (#1381, closed now).

@gabrieldemarmiesse
Copy link
Member Author

It's unfortunate this was overlooked during the reviews.

I'm working on making writing tests for addons easier and faster. See #1294 and #992 .

If it's easy for contributors to write more tests, we'll have more of them and this kind of issue won't happen again.

Copy link
Member

@Squadrick Squadrick left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@Squadrick Squadrick merged commit 7aac34c into tensorflow:master Mar 24, 2020
jrruijli pushed a commit to jrruijli/addons that referenced this pull request Dec 23, 2020
* Fix the serialization bug of rectified adam.

* Better error message.

* Update tensorflow_addons/optimizers/rectified_adam.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport r0.8 Will backport any PR merged with this label to the r0.8 branch cla: yes optimizers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lookahead bug when loading saved model
4 participants