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

Implement split real norm #241

Merged
merged 9 commits into from
Dec 14, 2021
Merged

Implement split real norm #241

merged 9 commits into from
Dec 14, 2021

Conversation

wdphy16
Copy link
Contributor

@wdphy16 wdphy16 commented Nov 22, 2021

As proposed in #196, it is an optimizer wrapper that splits the complex parameters into pairs of real parameters before sending them to the update of the wrapped optimizer, and merges the pairs of real updates into complex updates afterward.

@google-cla google-cla bot added the cla: yes copybara label for automatic import label Nov 22, 2021
@mkunesch mkunesch self-requested a review November 24, 2021 17:29
@mkunesch
Copy link
Member

Hi! Thanks a lot for the PR!
(Assigning myself - I have started reviewing and should have time to finish tomorrow.)

Copy link
Member

@mkunesch mkunesch left a comment

Choose a reason for hiding this comment

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

Hey!

Thank you so much for this PR and your detailed explanation in the gist attached to #196. This was super helpful for the review.

I think it looks great! I just have a few very minor comments and nits.

Thanks a lot again and let me know if you have any questions regarding the comments!

optax/_src/complex_valued.py Outdated Show resolved Hide resolved
optax/_src/complex_valued.py Outdated Show resolved Hide resolved
optax/_src/complex_valued.py Outdated Show resolved Hide resolved
optax/_src/complex_valued.py Outdated Show resolved Hide resolved
optax/_src/complex_valued_test.py Outdated Show resolved Hide resolved
optax/_src/complex_valued_test.py Outdated Show resolved Hide resolved
optax/_src/complex_valued_test.py Outdated Show resolved Hide resolved
optax/_src/complex_valued_test.py Outdated Show resolved Hide resolved
@wdphy16
Copy link
Contributor Author

wdphy16 commented Nov 26, 2021

Hi @mkunesch , thanks for your detailed review! I've made the changes accordingly.

@mkunesch
Copy link
Member

Looks great! Thanks for making the changes!

Copy link
Collaborator

@rosshemsley rosshemsley left a comment

Choose a reason for hiding this comment

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

Thanks! It's really great to see these additions :)

I left a bunch of thoughts to compliment @mkunesch 's review - I'll let him take over in managing getting this thing merged, though :)

optax/_src/complex_valued.py Outdated Show resolved Hide resolved
optax/_src/complex_valued.py Outdated Show resolved Hide resolved
optax/_src/complex_valued.py Outdated Show resolved Hide resolved
optax/_src/complex_valued.py Outdated Show resolved Hide resolved
optax/_src/complex_valued.py Outdated Show resolved Hide resolved
optax/_src/complex_valued.py Outdated Show resolved Hide resolved
optax/_src/complex_valued.py Outdated Show resolved Hide resolved
optax/_src/complex_valued_test.py Outdated Show resolved Hide resolved
optax/_src/complex_valued_test.py Outdated Show resolved Hide resolved
docs/api.rst Outdated Show resolved Hide resolved
* Change RealPair to NamedTuple
* Remove _is_real_pair
Copy link
Member

@mkunesch mkunesch left a comment

Choose a reason for hiding this comment

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

Thanks a lot @rosshemsley for the additional comments and @wdphy16 for implementing them!

optax/_src/complex_valued.py Outdated Show resolved Hide resolved
optax/_src/complex_valued.py Outdated Show resolved Hide resolved
optax/_src/complex_valued.py Outdated Show resolved Hide resolved
optax/_src/complex_valued_test.py Outdated Show resolved Hide resolved
@wdphy16
Copy link
Contributor Author

wdphy16 commented Dec 1, 2021

I've added the experimental namespace by mimicking haiku.experimental. Please see if you like it.

pylint reports that there are unused imports in experimental.py. How do you usually skip that?

@mkunesch
Copy link
Member

mkunesch commented Dec 1, 2021

Hi! Thanks a lot! It looks great to me! (Only flagged a missing full stop above)

Re the import error: I mean, we can easily disable it (for __init__.py it's disabled in the pylint settings afaik) but I'm also not sure why haiku adds a file for experimental.py rather than adding an experimental folder in _src and importing it in __init__.py. I assume that would work too and we wouldn't have to disable warnings then? (Also tagging @rosshemsley for an opinion on the structure).

@rosshemsley
Copy link
Collaborator

+1 to @mkunesch - I would go for

haiku/
   _src/
      experimental/
          complex.py

    __init__.py
        experimental/
              complex.py (or __init__.py)

@wdphy16
Copy link
Contributor Author

wdphy16 commented Dec 2, 2021

Ok, this project structure is also more intuitive to me

Copy link
Member

@mkunesch mkunesch left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Looks great to me. I like the folder structure and it passes the tests/lint checks.
Thanks a lot for the contribution again and for making the changes!

@wdphy16
Copy link
Contributor Author

wdphy16 commented Dec 13, 2021

Hi @mkunesch , shall we merge this now?

@copybara-service copybara-service bot merged commit 6bb8093 into google-deepmind:master Dec 14, 2021
@wdphy16 wdphy16 deleted the complex_optimizer branch December 14, 2021 15:47
@mkunesch
Copy link
Member

Hi! This got merged yesterday :-D! (Sorry it took slightly longer - it was blocked by a flaky test d663336).

Thank you very much for this PR again and for all your work (especially the excellent design doc!) on how to support complex numbers in optax!

@wdphy16 wdphy16 changed the title Implement split_complex Implement split real norm Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes copybara label for automatic import
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants