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

Standardized Testing for All Optimizers #2208

Merged
merged 3 commits into from
Nov 1, 2020

Conversation

hyang0129
Copy link
Contributor

References #992.

Aim is to create a single test file that tests all relevant classes within the optimizer module of tfa. The test will discover classes that inherit from tf.keras.optimizers.Optimizer then execute a set of standard tests on each of those classes.

This will test for common behaviors that optimizers should have (eg. serialization, minimizes, etc.).

@hyang0129 hyang0129 changed the title Standardized Testing for All Optimizers Standardized Testing for All Optimizers WIP Oct 18, 2020
@hyang0129
Copy link
Contributor Author

@seanpmorgan the optimizer module contains many classes that do not behave in the same way as an optimizer object. For example, multi_optimizer is a wrapper instead of an optimizer.

Perhaps we could have a class signature that exempts certain classes from the standard test?

Anyways, I've written some code to discover classes and it should work for any module. I think this can extend to layers and other modules that all contain classes that inherit from a common parent.

@hyang0129
Copy link
Contributor Author

Maybe we should have a list of behaviors that each class in a module should have. So for optimizers, I would imagine that list includes minimizes and serializes. Ironically, I feel like the process of defining tests for optimizers is defining what a tfa optimizer should be.

@hyang0129
Copy link
Contributor Author

on failure we get something like this.

standard_test.py FF [100%]

================================== FAILURES ===================================
____________________ test_optimizer_minimize[cpu-LazyAdam] ____________________

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.

Really like this and they way we can extend it to other subpackages as you mentioned. Couple of thoughts below.

tensorflow_addons/optimizers/tests/standard_test.py Outdated Show resolved Hide resolved
tensorflow_addons/optimizers/tests/standard_test.py Outdated Show resolved Hide resolved
updated list of optimizer exceptions and reasons
refactored naming of discover classes to make it more readable
@hyang0129
Copy link
Contributor Author

@seanpmorgan I've updated the code based on your feedback.

I've defined two standard behaviors for opts which is minimizes and serializes. I feel like there should be more standard behaviors but idk.

Also, half the classes appear to be wrappers. Once wrappers are standardized, we may need a way to select only optimizers and exclude wrappers because the testing criteria for wrappers could be different (or not because aside from wrapping, the wrapper should have the minimize and serialize behaviors. maybe just change how the test instances the wrappers?).

@hyang0129 hyang0129 changed the title Standardized Testing for All Optimizers WIP Standardized Testing for All Optimizers Oct 21, 2020
@hyang0129
Copy link
Contributor Author

Also, if we want to apply this to other modules, I think we should move the discover classes function to something more common like test_utils

@hyang0129 hyang0129 requested a review from seanpmorgan October 21, 2020 18:36
@hyang0129
Copy link
Contributor Author

@seanpmorgan

please review

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 @hyang0129! I agree with you that this could apply to more subpackages and so the discovery util could be moved into a more generic place.

Each subpackage will have their own exception list though so it could get a bit messy. For now I think this is good, but happy to consider a proposed refactor

@seanpmorgan seanpmorgan merged commit ec74bd7 into tensorflow:master Nov 1, 2020
jrruijli pushed a commit to jrruijli/addons that referenced this pull request Dec 23, 2020
* Added standardized optimizer serialization and minimize tests
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.

2 participants