Skip to content

Improve with providing Nuclear Norm Constraint #1107

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

Merged
merged 11 commits into from
Mar 22, 2020

Conversation

pkan2
Copy link
Contributor

@pkan2 pkan2 commented Feb 18, 2020

Hello!

I have updated the conditional_gradient optimizer with providing the option of nuclear norm constraint option, as described in issue #1105,

with using following formula:

variable -= (1-learning_rate) * (variable
    + lambda_ * top_singular_vector(gradient))

Thank you!

@pkan2
Copy link
Contributor Author

pkan2 commented Feb 21, 2020

Hello @facaiy @WindQAQ @Squadrick!

Can you provide some feedback for the update of CG optimizer?

Thank you!

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.

Most of the logic for fro and nuclear norm in _resource_apply_dense and _resource_apply_sparse is the same except for var_slice = tf.gather(...). Refactoring the logic into functions would make the code more succinct and easier to read.

Yet to properly review the test code.

@pkan2 pkan2 requested a review from Squadrick February 24, 2020 23:12
@googlebot

This comment has been minimized.

@gabrieldemarmiesse
Copy link
Member

@googlebot I consent

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@gabrieldemarmiesse
Copy link
Member

I've merged master into your branch to update it and fixed any formatting/conflicts it might have. If you need to do some more modifications, please do git pull beforehand.

@pkan2
Copy link
Contributor Author

pkan2 commented Feb 26, 2020

@gabrieldemarmiesse Got it! Thank you a lot!

@gabrieldemarmiesse
Copy link
Member

@Squadrick when you have the time, could you look into this pull request? Thank you.

@pkan2 pkan2 requested a review from Squadrick March 10, 2020 22:48
@Squadrick
Copy link
Member

@pkan2 There's a merge conflict, can you fix that? Once it's done, this should get merged.

@pkan2
Copy link
Contributor Author

pkan2 commented Mar 22, 2020

Hello @Squadrick . Thank you for your feedback! I have fixed the merging conflict.

Thank you!

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.

LGTM.

@gabrieldemarmiesse
Copy link
Member

I'll merge this as it was approved by @Squadrick

@gabrieldemarmiesse gabrieldemarmiesse merged commit 4e8c998 into tensorflow:master Mar 22, 2020
jrruijli pushed a commit to jrruijli/addons that referenced this pull request Dec 23, 2020
* Improve with providing Nuclear Norm Constraint

* Update based on feedback

* Update Based on Feedback

* Solve Conflict

* Solve Conflict

* change format and fix conflict

Co-authored-by: gabrieldemarmiesse <[email protected]>
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.

5 participants