Implementation of Conditional Gradient Optimizer

[pkan2]

Hello,

I have uploaded the implementation of Conditional Gradient optimizer into this directory. It is implementing the issue #468.

The contributors are Pengyu Kan and Vishnu sai rao suresh Lokhande, whose github name is lokhande-vishnu.

Sincerely,

Pengyu Kan

[Squadrick]

Could you rewrite the tests to be compatible with both graph and eager modes? Some of them seem to be graph only currently.

[Squadrick]

Could you update the PR after formatting as mentioned here.

[pkan2]

Hi @Squadrick! Thank you for giving me the link!

It is really helpful. But I have some error on "clang-format" while running the "make code-format". So I just manually run the yapf to change the format. Wish that it can work. Thank you!

[facaiy]

Hi, pkan2, did you try: bash tools/run_docker.sh -c 'make code-format' ? Note that it requires docker installed.

[facaiy]

Seems that we forget to append a comma in the line end :-)

FAIL: buildifier found errors and/or warnings in above BUILD files.
buildifier suggested the following changes:
12c12
<         "weight_decay_optimizers.py"
---
>         "weight_decay_optimizers.py",
Please fix manually or run buildifier <file> to auto-fix.
Bazel configuration format check fails.

[pkan2]

Hello @Squadrick and @facaiy! Thank you for your feedbacks!

I think the reason for failing the GPU test is caused by similar issue in #347. I am trying to use the solution provided in the this issue into the "conditional_gradient_test.py" file to get rid of the issue of "tf.half" in "'ResourceScatterUpdate' OpKernel" for GPU device. And it seems that "tensorflow_addons.utils" does not have attribute of "'GpuSupportsHalfMatMulAndConv'" from the solution. So I will probably just delete this part and keep the rest from the solution.

Thank you!

Sincerely,

Pengyu Kan

[Squadrick]

Yeah, that sounds good. We'll update the test once #347 is resolved. Could you add a TODO regarding this in the code?

[pkan2]

Hi @Squadrick! Thank you so much for the review! I have updated the file with the suggested changes. But for the variable name "lamda", I still have not decided on what name to choose, as "lambda" is a reserved key word in python. I will think about this point.

Thank you again!

[pkan2]

I have changed the variable name "lamda" into "Lambda".

[pkan2]

Hi @WindQAQ @facaiy ! Can you provide some reviews or suggestions on the modification for the code? Thanks!

Sincerely,

Pengyu Kan

[pkan2]

Hi! @WindQAQ @facaiy! Are there any more suggestions on the modifications I have to work on?

Thanks!

[facaiy]

Sorry for the delay, @pkan2 . I'll take a look today. Thanks for ping me, Dheeraj :-)

[Squadrick]

If you annotate ConditionalGradientTest with test_utils.run_all_in_graph_and_eager_modes, you don't need to again separately annotate each test* method with test_utils.run_in_graph_and_eager_modes.

[facaiy]

Thank you, Pengyu! I'll take a full review tomorrow :-)

[facaiy]

Can we use constraint here or other name? At least, lambda_ is accepted if we have no better choice :-)

[pkan2]

Thanks for the suggestion! It is just trying to keep the same letter used for expression in the formula by the paper. Can I choose "l_ambda" maybe? Otherwise, I can change into using "lambda_".

Thank you!

[Squadrick]

Thanks for your contribution! We'll get this merged as soon as the test cases are done.

[pkan2]

@Squadrick @facaiy Thank you a lot!

[facaiy]

Apologized that I'm busy this week than I expected. I'm fine to merge it if @Squadrick approved, and I'll recheck the PR later when I have time. 😄 thank all

[pkan2]

Hi @Squadrick @facaiy! I have changed the variable name of constraint into "lambda_". And I have updated the README file with adding co-contributor Vishnu Lokhande into it as maintainer too.

Thank you!

[Squadrick]

LGTM.