make eps a parameter of optimisers

[cossio]

Makes epsilon a parameter of each optimiser. Closes #1818.

PR Checklist

• Tests are added

[darsnack]

Only other comment is to call the field epsilon since that's the name we chose in Optimisers.jl (to avoid confusion with the function of the same name).

[cossio]

@darsnack I didn't want to change the value of eps Flux is currently using, so as to not change behavior. So I prefer not to use eps(typeof(eta)) as you suggest. I continue to use to the value of 1e-8 here.

Using eps(typeof(eta)) (which changes the value of epsilong used) is technically a breaking change so it could be adressed in a separate PR.

Also note that eta is a Float64 in all the optimizers, so using epsilon with type typeof(eta) is not really helping.

I renamed eps -> epsilon following your other suggestion.

[darsnack]

Fair enough, I didn't realize the type was hard coded to Float64.

[cossio]

Merge?

[darsnack]

bors r+

[bors]

Build failed:

• buildkite/flux-dot-jl

[darsnack]

Failure seems related to #1808 (@ToucheSir)?

[ToucheSir]

Yes, #1804 specifically. I still haven't been able to repro the issue locally (will try replicating the Buildkite env when I have time in a couple of days). Feel free to @test_skip that one if you need any PRs in sooner, otherwise help would be much appreciated on figuring out the CI failure.

[cossio]

Merge this? buildkite failure looks unrelated to this PR, and all other tests pass.

[darsnack]

@CarloLucibello @DhairyaLGandhi I think an org owner needs to merge.

[DhairyaLGandhi]

This was a bit too wide a change for the api and can break a bunch of code...

[darsnack]

can break a bunch of code

How so? It adds an optional positional argument at the end that has the same default value as before.

[DhairyaLGandhi]

It adds a field to the optimisers, any code using the default "struct" constructors would break.

[ToucheSir]

Sorry, I should've caught that after #1778. Shall we quarantine both off for 0.13?

[DhairyaLGandhi]

#1778 is fine still because the disruption is limited to one type with a clear way of making a case for a non breaking type.

[cossio]

Sorry I didn't realize that. To be clear, what you mean is that a call like:

RMSProp(η, ρ, IdDict())

no longer works, because it now needs to be replaced by:

RMSProp(η, ρ, ϵ, IdDict())

right?

A possible solution is to make another PR (overwriting this one), which makes ϵ an optional keyword argument. Would that work?

RMSProp(η, ρ, IdDict(); ϵ = ϵ)

[DhairyaLGandhi]

Right

…

On Wed, Dec 29, 2021, 23:15 cossio ***@***.***> wrote: Sorry I didn't realize that. To be clear, what you mean is that a call like: RMSProp(η, ρ, IdDict()) no longer works, because it now needs to be replaced by: RMSProp(η, ρ, ϵ, IdDict()) right? A possible solution is to make another PR (overwriting this one), which makes ϵ an optional keyword argument. Would that work? RMSProp(η, ρ, IdDict(); ϵ = ϵ) — Reply to this email directly, view it on GitHub <#1819 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJOZVVLO32YVQXESJYSF6JLUTNCNFANCNFSM5K4BPGPA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: <FluxML/Flux .***@***.***>

[ToucheSir]

Well, I'm still not sure if adding struct fields constitutes a SemVer break, as (de)serialization (among other things) is affected. If anyone is in the know about this, please do leave a comment.

[darsnack]

@DhairyaLGandhi thanks for the explanation, sorry I missed that.

How frequently is the IdDict manually being passed in though? I'd rather have a breaking change for v0.13 than complicate the API by adding a keyword just to make it not breaking.

[DhairyaLGandhi]

It's got less to do with frequency and more about necessity and API compactness. In cases where we need literal copies of gradients (for synchronisation for example) over different models it's helpful to store states and gradients as the same type as literal Dicts for simplicity, and making users add in an epsilon fiels (especially when not needed in the optimisation step) is unnecessary.

[darsnack]

I get that, but I'm saying if the frequency is low enough (i.e. a handful of users), then it's fair to ask that handful to add the new field into their code on a breaking release. Anyone manually passing in the IdDict is already passing in a bunch of parameters other than the LR. This seems reasonable to me in order to not have a global constant in the package affecting the behavior of many functions.

[cossio]

How frequently is the IdDict manually being passed in though? I'd rather have a breaking change for v0.13 than complicate the API by adding a keyword just to make it not breaking.

I don't see why making epsilon a keyword would complicate the API? It would be an optional keyword that most users need not even notice.

[DhairyaLGandhi]

Passing in Dict explicitly is more than passing a bunch of arguments though, it is arguably more related to the use case of the optimisers than the value of an epsilon. It's already very rare to need to change epsilon.

[darsnack]

I meant that if you are manually passing in the dict, then you are already writing ADAM(lr, (0.9, 0.999), IdDict(...)). No one changes the (0.9, 0.999) either, but these are extra arguments the user is copy-pasting in order to pass in the IdDict. So having to update the code to ADAM(lr, (0.9, 0.999), 1e-8, IdDict(...)) on a breaking release seems okay to me.

[DhairyaLGandhi]

But even a long list of arguments to the constructor maintains a rational order (going by what is likely to be used and requires changing for the majority of use cases), which epsilon in the position in the pr does not since it comes before the storage. It should be after.

[darsnack]

Sure, a PR to fix the order is okay for me.

[ToucheSir]

I would favour grouping the hparams together. Perhaps if the IdDict was the first parameter in the default constructor?

[cossio]

I'd prefer

RMSProp(η, ρ, IdDict(); ϵ = ϵ)

over

RMSProp(η, ρ, IdDict(), ϵ)

In fact, having all hyper-params as keyword arguments wouldn't be bad I think,

RMSProp(IdDict(); η=η, ρ=ρ, ϵ=ϵ)

but that's another matter.

[ToucheSir]

For ADAM at least, I'd argue β is not sufficiently more worthy of tuning than ϵ as to warrant being before the IdDict than after. Therein lies the challenge though: can we agree on which hyperparams are "privileged" and which are relegated to kwargs? If not, putting the IdDict first by convention prevents (most) any bikeshedding there.

[cossio]

Any decision here?

[DhairyaLGandhi]

I prefer the first proposal in #1819 (comment). Since ϵ is common amongst many optimisers, and fitting it into different slots of optimiser API might get meddlesome, having it consistently as a kwarg makes sense. Besides, while experimenting with the hyper parameters of ADAM, I'd imagine modifying β would happen more frequently than ϵ (which strictly may not be a parameter at all).

[cossio]

I prefer the first proposal in #1819 (comment).

I agree. I can make a PR.