Nit: Why not set a default value similar to other optimizers?

I agree, 1e-3 is being used on the others 👍

Do you mean to set a default value for the learning rate? I didn't because SGD, Adam, and other optimizers don't have one.

Adam has lr=1e-3 as @frgfm mentioned. I agree it would be good to have a default here.

func is not declared if scripting. Is this implementation complete?

Only the single_tensor implementation is complete. The multi_tensor one is still missing.

I'm temporarily raising an exception when scripting.

EDIT: I looked at the SGD implementation more carefully, and it seems that just scripting + foreach is not supported. So, I think that the original implementation was OK.

Nit: why not put the trust_coefficient and eps in the defaults above?

Fixed.

Does it make sense to grab these from the group similar to other params here?

Yes!

I agree, 1e-3 is being used on the others 👍

I suggest keeping the same arg order as other optimizers as well

I followed SGD's order; Adam's is quite different (due to different parameters too).

Open suggestion but perhaps list comprehensions would be better?

Using a list comprehension I would need to loop more than once. In this way, I loop only once, which is more efficient.

For readability, I suggest moving that before the functional API call

I don't think it would be the same: in the functional call, momentum_buffer_list is updated in place.

Agreed it would be after

the global LR is not applied to the momentum part to the best of my understanding

Thank you, I will check again the paper and some online implementations soon.

Not sure the eps is required: if p_norm is zero, then the whole thing is zero, if g_norm is zero, then the basic update term is zero (same effect as using local LR = 1 if any of those is zero, but we avoid the eps imprecision :))

I think it's required to avoid zero division when both terms are zero.

p_norm and g_norm are banned from being 0 with the conditional, no?

That said, I am not sure why lightning included an eps.

This would avoid unnecessarily biasing the denominator.

I checked the code again, and I think you're right.

I notice that has_sparse_grad is excluded from this implementation so far. What are your plans with the variable?

Wanted to give you the heads up that we are planning on deprecating has_sparse_grad, because our main use of it is to determine whether we could use the foreach implementation or not for it. Instead of barring all params from going through the foreach implementation because of one sparse grad, the new ideal is to group the tensors by device and to put everything that cannot be foreach'd into the cpu bucket, so that we can maximally enjoy the speed of foreach.

To be honest, I skipped it. My idea was to complete at least the basic implementation, then gradually implement more "advanced" parameters, like foreach.

Does it sound good to you?

yes, I wanted to give you more context regarding the deprecation of has_sparse_grad (as we plan to remove it for all optims as well)

Add default 1e-3 here

Unless with LARS it is recommended to decide on an initial LR? From the paper, it looks like they specifically use bigger LRs.

Yes, they experiment way bigger LR in the paper. That's why I didn't initially set a default value.

Here, LR=1.0 is set, whereas no default value is set here.

Ah, maybe we should just up the default to 1.0, with a comment that larger LRs are used with LARS

We would want to include the latex version of the algorithm here. I just took some time to read the arxiv linked for the LARS introduction. Is it just me or are there several key typos in their algorithm (e.g., the trust coefficient should be used in the local lr update but it isn't?)

I noticed it too.

Do you think I should just copy the algorithm they reported in the paper?

.data is deprecated + should not be used

This should be a cheaper check

This should be a cheaper check

Actually the check is probably unnecessary (reason being that they'll be floats and comparing floats to 0 is often silly). I would vouch for getting rid of this check entirely and keeping the eps term.

I moved the two norm computations inside the if condition, since they are used only there.

avoid inplace updates

Ok. There're some other in-place updates. Should I change them too?

I'm referring to:

and

Which two are you referring to here? The optim tests are certainly up for a refactor, so there might just be actual redundancies we can get rid of

I'm refererring to the first two test cases defined here