It looks like it's just the flake test complaining about extra whitespace.
And the way to fix the failing "equivalent to lua torch"-test, I guess is to fix it in lua torch first? :)

Or just remove the weight decay test in Adam from https://github.com/pytorch/pytorch/blob/master/test/optim/tests.json if the devs are OK with giving up lua torch compatibility on that point.

@HaraldKorneliussen thanks! Yes that is the question, if I should also change the legacy code or remove the test?

I would like to see this merged, as I think that paper's result was an important one. I also prefer "fixing" the existing optimizers as in this PR over the huge copy-paste in the other PR.

As it currently is implemented, I have never encountered a case where weight-decay has helped me in any of the non-SGD/Momentum optimizers, nor has anyone I have talked to.

That being said, we might want to keep the original behaviour by default, at the very least for SGD/Momentum. Imagine all the training codes where the performance changes (for the better or the worse) after an update, and how near-impossible it would be to find out that this PR is why! Then, read the paper, and fix the factors accordingly. So I'd rather see a new flag for it, something like separate_weight_decay=False by default.

Or maybe print a (one-time per session) warning whenever the optimizer is created with non-zero weight_decay for the next handful of versions.

I'm a bit concerned about this PR, because it completely changes how optimizers work, and may hurt reproducibility. Any opinions @soumith?

I agree @apaszke it will cause issues with reproducibility. What is the pytorch design principle? to stay backwards compatible? I could add a flag that switches from default to this behaviour as @lucasb-eyer suggests...

@apaszke you are right in that it would be different from most (all?) popular frameworks. I haven't seen enough confirmation of its effectiveness in the wild (have you, @kashif?) so maybe the conservative separate_weight_decay=False is the better thing to do here. But no matter how, I'd really like to have this in PyTorch.

Or be the bold Apple of DL and move to Doing It Right(TM), just with a big fat warning as I mentioned here 😄

@lucasb-eyer as far as I am aware only chainer head has this implemented currently for the adam optimizer by default. No other optimizers have weight decay so they have not added it to them, like I did here...

In terms of experiments, the ones we ran for chainer are here:

This implementation doesn't follow the paper Fixing Weight Decay Regularization in Adam. This pull request applies weight decay using

p -= weight_decay * p

whereas the paper proposes applying weight decay using

p -= lr * weight_decay * p

c.f. Algorithm 2 on page 3.
Is there a strong reason for not following the paper's proposal to the letter?

@kashif the problem with weight decay was that it was applied before the smoothing stuff, not that weight_decay was getting mixed up with the learning rate.

@jpeg729 the pytorch api makes it seem like I am not multiplying the learning rate, especially the inplace APIs, but if you see the api call bellow that, then you will see that the learning rate gets multiplied etc. as in the paper... Thanks and hope it helps!

@kashif I beg to differ.
For example, from your modified code for SGD. I have added comments according to my understanding of the pytorch inplace apis.

            if weight_decay != 0:
                p.data.add_(-weight_decay, p.data) # p.data = p.data + weight_decay * p.data

            p.data.add_(-group['lr'], d_p) # p.data = p.data + lr * d_p

Only d_p gets multiplied by the learning rate, not the weight_decay update.
As far as I can see, none of the optimizers multiply the weight_decay by the learning rate.

@jpeg729 ok so I fixed the negatives:

if weight_decay != 0:
    p.data.add_(-weight_decay, p.data) # p.data = p.data - weight_decay * p.data

p.data.add_(-group['lr'], d_p) # p.data = p.data - lr * d_p = p.data -lr * d_p - weight_decay * p.data

which is essentially line 9 in Algorithm 1 of the paper. Could it be that you are thinking of the eta_t in the paper as the learning rate? eta_t in the paper is the learning rate schedular which here is just 1.

@kashif You are right. eta is the Scheduler.
But, I think that there is a problem here that requires some thought.
In the paper at time t the effective learning rate is eta * alpha, and the effective weight_decay is eta * w.

So if someone uses a learning rate scheduler with these optimisers, then they have to manually apply a similar scheduler to the weight decay in order to replicate the methods in the paper. Otherwise the grad updates would decay but not the weight decay updates, and eventually the grad updates would be swamped by the weight decay updates.

One fix would be to add param_group['eta'] to each optimiser and change the schedulers to modify param_group['eta'] instead of param_group['lr'].

@jpeg729 yes you are right, at the moment the schedular will only change the learning rate and not the weight decay parameter... let me see how hard it will be to add that.. but as you can read from the comments above I dont think these changes are going to be accepted so not sure how much its worth investing time on this... I might close this PR soon.

@kashif if this PR does not get merged (likely), do you plan to create a separate repo that trackes optim but with these changes? Something like optimw, or will you just let it die and not use these changes anymore yourself?

/cc @loshchil

@jpeg729 As a workaround using the current code here, will it correctly apply scheduling on the weight decay if I set

weight_decay = actual_weight_decay / initial_lr
and correspondingly, in the optimizer,
p.data.add_(-group['weight_decay'] * group['lr'], p.data)

Such that the initial learning rate get cancelled out and is left with eta?

I didn't try it but I think that you are right @hkchengrex , this should work and implement eta as described in the paper. Otherwise, the current code has the issue described above by @jpeg729
At first, it might seem unusual that weight decay is scheduled. However, a closer look at the original SGD with L2-based weight decay will reveal that weight decay is scheduled there as well because the weight decay part is first combined with the loss-based gradient and then multiplied by learning rate which is usually scheduled.
Please note note that the optimal value of weight decay in SGDW is likely to be different by a factor of lr from the one of SGD with L2, see Section 2 of https://arxiv.org/pdf/1711.05101.pdf.

cc: @vincentqb can you review this and get it to completion if it makes sense. Use the guidelines that I shared with you separately.

@kashif i want to apologize for never getting a review to completion, but we now have Vincent with a lot of bandwidth and who is a math and optimization expert. He will help with the review.

thanks @soumith I understand the delay. I will try to work with @vincentqb to get it ready for another review!

@soumith @vincentqb i see you have merged versions of this optimizer without reviewing my PR pending for more than a year, as you requested 17 days ago in the comment above. I find that disrespectful even if i understood the limited bandwidth you have but to then go ahead and accept copies of this work is unfair.

@soumith @vincentqb
It seems that the new code does not take into account the implementation issue discussed in the current PR by @kashif (e.g., see my comment above and also here ). In might be necessary to make users aware that the implemented algorithm only partially follows the paper (it follows the main contribution of the paper that the weight decay update is taken out from the adaptive gradient update but the 'lr' and 'weight_decay' hyperparameters are still coupled in contrast to the paper where a scheduler is used).

I would like to thank you all and especially @kashif for investing your time to disseminate this work.

@kashif it was disrespectful, I fully agree -- sorry about that. It's also not a situation we wanted to be in -- where multiple PRs were pending for the same algorithm, and we had no maintainers for optim for like forever. If I could undo it one way or the other I would. I'm banking on @vincentqb being a maintainer who will over time do better (he is still ramping up, and I'm sure he doesn't have full context yet).

@vincentqb can you look at what @loshchil mentioned.

@soumith thank you for your kind message and i accept your apology and apologise for my outburst. I too will try to be more understanding. Sleeping over it I realised there was something else bothering me that I needed to work through. Anyways, let me know if I can help in anyway.

@kashif Thank you for your understanding. I appreciate your contributions -- and everyone else's in this pull request -- and I also want to apologize. I was in the process of going through this pull request.

The two key contributions in AdamW and SGDW seem to be:

1. Weight decay done through the parameter theta update directly (instead of through the parameter gradient g: line 6 vs 9 in algorithm 1, line 6 vs 12 in algorithm 2),
2. The definition of a new ScheduleMultiplier eta(t) replacing LRScheduler lr(t). In that case, the implied relation between ScheduleMultiplier and LRScheduler is lr(t) = lambda*alpha*eta(t), with lambda and alpha two constants.

Based on the discussions above, I am also understanding that we could update lambda and alpha independently instead of eta. Thus effectively having two separate schedulers, one for the momentum update and one for parameter update. This would be why 2 above is referred to as "decoupled weight decay". Is this what you mean @loshchil ?

If that is the case, my initial plan was the following. Given that we do not have a separate weight scheduler concept yet #22343 for the momentum update, the initial plan was to bring in AdamW and SGDW with "coupled" learning rate and weight decay (and thus using the same scheduler). However, based on the discussion above, the name AdamW and SGDW implies "decoupling" to the user, and not having the decoupling would bring confusion.

Thoughts?