Fix ADADelta calculations and broken tests not catching the problems

[scimas]

29832ac broke ADADelta, simply reversing its change and adding comments to not move around the epsilons. Fixes #1158

The testset probably needs more work than specifically for this case, so I'm not adding it here.

PR Checklist

• Tests are added fixed
• Entry in NEWS.md
• Documentation, if applicable
• Final review from @dhairyagandhi96 (for API changes).

[CarloLucibello]

thanks! can we fix tests so that we would have catched this?

[scimas]

I kind of found why the test isn't working, but I have no idea how it is happening.

Flux.jl/test/optimise.jl

Lines 6 to 13 in 1dc9023

	@testset "Optimise" begin
	w = randn(10, 10)
	@testset for opt in [ADAMW(), ADAGrad(0.1), AdaMax(), ADADelta(0.9), AMSGrad(),
	NADAM(), RADAM(), Descent(0.1), ADAM(), OADAM(), Nesterov(), RMSProp(),
	Momentum()]
	w′ = randn(10, 10)
	loss(x) = Flux.Losses.mse(w*x, w′*x)
	for t = 1: 10^5

If you add an @info loss(rand(10)) right after the loss function definition and before the for loop, it prints a loss of 0.0. Which means it hasn't been testing anything for who knows how long.

If you remove the @testset from @testset for opt in [...] it works correctly and reports non zero loss and that ADADelta is failing. I haven't checked other optimisers yet. Also, I don't know whether removing the @testset is the right way to go forward.

[scimas]

Alright, figured it out. Every @testset execution resets the global RNG's state. Which means w == w′ at all times. I will add an explicit seed set statement and see whether the tests pass.

[DhairyaLGandhi]

Locally did you verify/ cross check if there are other optimisers affected by this?

[scimas]

Are you talking about the tests not doing anything or any other optimisers being affected by the epsilon like changes?

The tests should be fixed, I think. All of the optimisers are being tested in the same @testset, so the fix should apply to all of them.

I haven't checked other optimisers for algorithmic correctness. That would be next.

[scimas]

Looks like OADAM might be broken, the test failed for only that one.

[scimas]

Calculations of OADAM look fine to me. It does seem to have reduced the loss to a certain extent, just not as much as the @test is expecting (OADAM is taking it to ~0.025, test expects 0.01). Even giving it 10 times more iterations doesn't seem to show much improvement. Maybe OADAM is just not good at "optimising" random numbers (wild guess, not an optimisation theory expert)?

Perhaps changing the test to just check that the loss has decreased would be a good idea (as opposed to checking it has reduced below a hardcoded value of 0.01). Please provide other suggestions too.

Also should that be a separate PR? This isn't specific to fixing ADADelta anymore, it is already fixed.

[DhairyaLGandhi]

It might mean that the default parameters of the optimiser aren't ideal.

[scimas]

That is a good possibility. The paper itself does not suggest any default parameters, only tells which ones they used when conducting their experiments. On top of that, neither PyTorch nor Tensorflow have implemented OADAM, so that avenue of finding established defaults is closed too.

[scimas]

Increasing the learning rate of OADAM by a factor of 10 is enough to make it pass the test. Should we change the default parameter value or only increase in the test?

[DhairyaLGandhi]

The default one would be best, thanks!

[scimas]

Ready for review

[DhairyaLGandhi]

Thanks! Lgtm

bors r+

[bors]

Build succeeded:

• ci/gitlab/gitlab.com