optax icon indicating copy to clipboard operation
optax copied to clipboard

Add sophia-h optimizer

Open evanatyourservice opened this issue 1 year ago • 10 comments

PR to add sophia optimizer. It's mostly based on levanter's implementation with some changes/added features here and there.

One note is that I had to change the contrib common test file a couple times, once to pass the loss_fn out of the parabola and rosenbrock functions (could be useful later for other optimizers that need loss function), and a second time to bypass the check for update arguments to be values (the loss function is not). Please advise if these changes are not ok or the most correct.

evanatyourservice avatar May 29 '24 19:05 evanatyourservice

fixes #968

evanatyourservice avatar May 29 '24 19:05 evanatyourservice

Hi Vincent, thank you for the notes! They all make perfect sense to me and I'll get to updating the code/answering them tomorrow

evanatyourservice avatar Jun 14 '24 21:06 evanatyourservice

@evanatyourservice please ping us whenever you're ready for another round of reviews :-)

fabianp avatar Jun 27 '24 10:06 fabianp

@fabianp Will do! Sorry been moving but will try to get this going asap

evanatyourservice avatar Jun 27 '24 15:06 evanatyourservice

there's no rush, just wanted to make sure you were not waiting on us :-)

fabianp avatar Jun 28 '24 09:06 fabianp

@vroulet @fabianp Got some updates pushed, let me know if anything needs to be changed! Thanks

evanatyourservice avatar Jun 28 '24 16:06 evanatyourservice

Hello @evanatyourservice, Sorry for the very long delay on our end. I think your code looks great! If it's ok with you, can you merge the code with head once #1060 is merged (#1060 adds more tests for the contrib optimizers to ensure compatibilities). Then I should approve and finish on our side if there are still minor details to fine-tune. Thank you again!

vroulet avatar Sep 14 '24 01:09 vroulet

sounds good!

evanatyourservice avatar Sep 16 '24 18:09 evanatyourservice

Hello @evanatyourservice, #1060 got merged. You may merge the tests, address Fabian's comment, and I can approve (and maybe fix minor issues on our end if there are). Thank you again!

vroulet avatar Sep 23 '24 17:09 vroulet

Ok sounds good! Sorry I should get to this tomorrow

evanatyourservice avatar Oct 01 '24 00:10 evanatyourservice

@fabianp @vroulet wow ok 1 day turned into 3 weeks, let me revert rademacher to normal and make sure the tests are all good and I'll push update :)

evanatyourservice avatar Oct 22 '24 16:10 evanatyourservice

gentle ping @evanatyourservice :-)

no rush but it would be a really nice feature to have landed in optax ❤️

fabianp avatar Dec 04 '24 10:12 fabianp

Thank you again @evanatyourservice ! We added tests and made some final fixes necessary for the tests to pass.

vroulet avatar Dec 05 '24 18:12 vroulet

Awesome! Thank you for finishing it up! I got kinda stumped at some of the tests and was debating another strategy but then it fell out of my mind for other tasks. Glad we were able to get it merged!

evanatyourservice avatar Dec 05 '24 18:12 evanatyourservice