Implement DeltaBarDelta using refactored GradientDescent.

[ranjodhsingh1729]

Closes: #437

This PR:

• Implements DeltaBarDeltaUpdate
• Refactors GradientDescent to GradientDescentType<UpdatePolicyType, DecayPolicyType>
• Defines DeltaBarDelta as a typedef to GradientDescentType<DeltaBarDeltaUpdate, NoDecay>

GradientDescent is now a typedef to GradientDescentType<VanillaUpdate, NoDecay>.

[ranjodhsingh1729]

Hi,
would it make sense to include a change here which allows PrintLoss callback to work with GradientDescentType (since ProgressBar() would require more invasive changes). It doesn't currently as PrintLoss prints when EndEpoch is called and GradientDescentType never calls EndEpoch.

[ranjodhsingh1729]

@conradsnicta could you please approove the CI build i can't test this with bandicoot locally.

[zoq]

Had to restart the GPU runner, restarted the jobs.

[ranjodhsingh1729]

Thanks @zoq
The Tests ran successfully.

[zoq]

Fantastic, can you also update the HISTORY.md file.

[ranjodhsingh1729]

Hi

Fantastic, can you also update the HISTORY.md file.

Done.

Hi, would it make sense to include a change here which allows PrintLoss callback to work with GradientDescentType (since ProgressBar() would require more invasive changes). It doesn't currently as PrintLoss prints when EndEpoch is called and GradientDescentType never calls EndEpoch.

What do you think about this.

Additionally I found this small inconsistency.
If maxIterations is non-zero it will perform one less iteration.

ensmallen/include/ensmallen_bits/gradient_descent/gradient_descent_impl.hpp

Line 70 in 9aebc1d

for (size_t i = 1; i != maxIterations && !terminate; ++i)

Should I go ahead and fix these here? OR seperately?

[ranjodhsingh1729]

In general, fixes are orthogonal to new features, and should be kept separated.

Thanks @zoq for the great advice.
It made sense the moment i read it. :)

I think this is ready for review then.
This isn't urgent or anything whenever you(or anyone else) can find the time will be good.

[rcurtin]

It is definitely not okay to merge an optimizer that has not been thoroughly reviewed. I will get to it when I have a chance and there is no schedule on when that will be.

[rcurtin]

Thanks for the contribution @ranjodhsingh1729. I have some concerns about the correctness of the implementation---can you look into the comments I left? Let me know if I can clarify anything, or if I missed anything. I used primarily section 6 of the paper for my review.

[rcurtin]

Suggested change

	* [Increased rates of convergence through learning rate adaptation](https://www.academia.edu/download/32005051/Jacobs.NN88.pdf)
	* [Increased rates of convergence through learning rate adaptation (pdf)](https://www.academia.edu/download/32005051/Jacobs.NN88.pdf)

Most of the other links indicate to people when a PDF is going to be opened/downloaded.

Also it's probably worth linking to GradientDescent here too.

[rcurtin]

Because DeltaBarDelta always uses the DeltaBarDeltaUpdate class, it doesn't really make sense to the user to provide a constructor that takes updatePolicy, decayPolicy, and resetPolicy. Of course an advanced user can do this if they take a look at the internals of the GradientDescent class. But for the typical user, they just want to set the settings of DeltaBarDelta and move on. So I would suggest adding a wrapper class DeltaBarDelta that just calls GradientDescentType<DeltaBarDeltaUpdate, NoDecay> internally (there are many of these for SGD variants), and also provides a constructor that forwards the parameters of DeltaBarDeltaUpdate:

DeltaBarDelta(stepSize, maxIterations, tolerance, kappa, phi, momentum, minGain, resetPolicy)

Then the table of options below will be much simplified too.

[rcurtin]

Suggested change

	const bool resetPolicy)
	: stepSize(stepSize), maxIterations(maxIterations), tolerance(tolerance),
	updatePolicy(updatePolicy), decayPolicy(decayPolicy),
	resetPolicy(resetPolicy), isInitialized(false)
	const bool resetPolicy) :
	stepSize(stepSize),
	maxIterations(maxIterations),
	tolerance(tolerance),
	updatePolicy(updatePolicy),
	decayPolicy(decayPolicy),
	resetPolicy(resetPolicy),
	isInitialized(false)

Just a style fix.

[rcurtin]

Suggested change

	* @file delta_bar_detla_test.cpp
	* @file delta_bar_delta_test.cpp

[rcurtin]

Based on my read of Eq. (4) in the paper I'm not sure this is correct. The value of gains[i] should not change if (gradient % velocity)[i] is 0; but given how mask is set, I can't quickly see that this is correct.

I would suggest maybe using sign() instead to create a matrix indicating whether the element is positive, negative, or zero, and then use that matrix to apply the update rule in (4).

[rcurtin]

But the paper seems to indicate we update with ϵ(t + 1) in Eq. (3), not ẟ'(t)?

I would suggest using the same terminology as the paper throughout even if it is not intuitive what ẟ', ϵ, and θ actually mean---you can clarify that part in the comments.

[rcurtin]

How does this implement the exponential average ẟ'(t) = (1 - θ) ẟ(t) + θ ẟ'(t - 1)? This doesn't appear to be that.

[ranjodhsingh1729]

@rcurtin, Thanks a lot for the thorough review - turns out i've made quite a mistake here.

Since both t-SNE papers (the original and the tree-based approximation) state using Jacobs's scheme and cite this paper, I assumed it was used as-is and didn't verifying that assumption.

Now i know that what is used in t-SNE was a modified version of the DBD rule that incorporates momentum, while the original DBD only includes momentum for 'comparison' and doesn't apply it in updates.

Glad this was caught during review - definitely a reminder not to rush things like this in the future. 😅

Since there's already an open issue, PR, and review, I've gone ahead and implemented the actual delta-bar-delta rule and also addressed the review suggestions.

So I guess this puts me back at square one - I'll need to implement the t-SNE update rule in mlpack and hook it up with the refactored gradient descent class. Since it's not really used elsewhere, adding it to ensmallen probably doesn't make sense. Does that sound right, or is there a cleaner way to handle it?

And sorry for the confusion here.

[rcurtin]

I think it's just fine to add Delta-Bar-Delta with momentum here. It would add to the list of optimizers that ensmallen supports, no problem at all with that even if the target use (for now) is just t-SNE. You could either:

• add options to the existing DeltaBarDelta optimizer for momentum
• add a new MomentumDeltaBarDelta optimizer rule that is DeltaBarDelta with momentum added

I'm not picky about which, though I might lean very slightly towards the second option, since we have separated SGD and MomentumSGD.

[ranjodhsingh1729]

I should have clarified this in my last response: tsne adds momentum yes but it also makes some further modifications:

Here is a little snippet showing both:

"""Assumes epsilon is initialized with step_size"""
def dbd():
    sameSign = delta_bar * delta > 0.0
    diffSign = delta_bar * delta < 0.0

    epsilon[sameSign] += kappa
    epsilon[diffSign] -= phi * epsilon[diffSign]
    np.clip(epsilon, min_gain, np.inf, out=epsilon)

    delta_bar = theta * delta_bar + (1 - theta) * delta
    Y -= epsilon * delta

"""
Assumes delta_epsilon is initialized with zeros
Effective step_size = step_size * delta_epsilon

diffSign => increment epsilon, sameSign => decrement epsilon
This is opposite of what the dbd does.

decrement is by (1 - phi) * delta_epsilon (written here as *= phi)
whereas it was by phi * epsilon in original dbd

no delta_bar is present here
velocity is used for comparison
"""
def tsne():
    sameSign = velocity * delta > 0.0
    diffSign = velocity * delta < 0.0

    delta_epsilon[diffSign] += kappa
    delta_epsilon[sameSign] *= phi
    np.clip(delta_epsilon, min_gain, np.inf, out=delta_epsilon)

    velocity = theta * velocity - step_size * delta_epsilon * delta
    Y += update

These changes are not equivalent to simply adding momentum to the final update like this:

"""Assumes epsilon is initialized with step_size"""
def momentum_dbd():
    sameSign = delta_bar * delta > 0.0
    diffSign = delta_bar * delta < 0.0

    epsilon[sameSign] += kappa
    epsilon[diffSign] -= phi * epsilon[diffSign]
    np.clip(epsilon, min_gain, np.inf, out=epsilon)

    delta_bar = theta * delta_bar + (1 - theta) * delta
    velocity = theta * velocity - epsilon * delta
    Y += velocity

So essentially tsne just uses the idea behind dbd with momentum.
Now given this i am not sure if implementing it under MomentumDeltaBarDelta would be okay. Let me know what you think!

[rcurtin]

I see what you mean, thanks for the clarification. I haven't spent enough time with the t-SNE paper yet to understand exactly why they chose to do things differently but I will get to that when I have a chance. In the mean time it sounds like the route to a solution is to use two different update rule classes for each optimizer. I guess MomentumDeltaBarDelta works for the t-SNE version but if you have a better idea for a name I'm all ears. (The thought enters my mind of whether the paper has a typo in the algorithm description---that definitely does happen more than one would hope for peer-reviewed papers. I'll think about it more as I have a chance to jump in.)

[ranjodhsingh1729]

The thought enters my mind of whether the paper has a typo in the algorithm description

Both the papers (the original and the tree-based approximation) stated using "Jacob's scheme". I just assumed (mistakenly) it was dbd since dbd was the only thing that looked similar in the cited jacob's paper.

Papers didn't mention any modifications those were only given in the implementation given on lvd's page

[rcurtin]

That's totally fine, like I said, we can implement both as separate rules. That's probably the best thing to do here since you already took the time to get the original DBD rule ready. Does that work for you?

[ranjodhsingh1729]

Sure i'll implement the other rule under MomentumDeltaBarDelta as you suggested (I don't see any other way either).

[ranjodhsingh1729]

Sorry for the delay in getting to this. I've noted a few things i am not entirely sure about:

In DeltaBarDeltaUpdate
The class is passed with initialStepSize in order to fill the epsilon matrix. Another solution could be:

diff --git a/compare.1 b/compare.2
index 673c847..fef8c17 100644
--- a/compare.1
+++ b/compare.2
@@ -3,11 +3,11 @@
       const MatType diffSignMask = conv_to<MatType>::from(signMatrix == -1);
 
       epsilon += sameSignMask * kappa;
-      epsilon -= diffSignMask * phi % epsilon;
-      epsilon.clamp(minStepSize,
+      epsilon -= diffSignMask * phi % (stepSize + epsilon);
+      epsilon.clamp(minStepSize - stepSize,
           arma::Datum<typename MatType::elem_type>::inf);
 
       delta_bar *= theta;
       delta_bar += (1 - theta) * delta;
 
-      iterate -= epsilon % delta;
+      iterate -= (stepSize + epsilon) % delta;

In MomentumDeltaBarDeltaUpdate
Since actual stepSize used for update is stepSize * gains, gains had to be clamped this way. Another way could be to take minGain as input as done previously.

      // stepSize * gains >= minStepSize
      // gains >= minStepSize / stepSize
      gains.clamp((double) minStepSize / stepSize,
          arma::Datum<typename MatType::elem_type>::inf);

[rcurtin]

For the first thing, I'm not sure what the advantage of the second approach is; for DeltaBarDelta, the user specifies a step size in the constructor. This is the initial step size of the entire optimization, which is then tuned and updated by the rule that you've implemented. So I don't see what the purpose of the extra changes are. I think it makes more sense with the a/compare.1 implementation.

For MomentumDeltaBarDelta, I'm not sure I understand the full question: I agree with what you wrote, but is this really a question about the user interface and what options are available? The original MATLAB implementation gives parameters that are the initial step size (e.g. initial epsilon value) and the minimum gain, so I might lean towards exposing those parameters for MomentumDeltaBarDelta instead of a different approach.

I'm not 100% sure I understood the questions right, so please clarify and point me in the right direction if I didn't. :)

[ranjodhsingh1729]

@rcurtin
Thanks for the detailed response. You are correct on both counts. My questions were just for confirmation and/or input. I've changed minStepSize to minGain in MomentDeltaBarDelta. Another review at your convenience would be appreciated (no urgency).

[rcurtin]

Hey @ranjodhsingh1729, sorry that this took me so long to get back to. I verified the implementations using the original Delta-Bar-Delta paper and the MATLAB t-SNE reference implementation. Everything looks good implementation-wise. I have a whole bunch of other comments but they should be pretty easy and quick to handle. If anything I wrote is wrong don't hesitate to call me out on it---you've spent more time in the details of these algorithms than I have. Anyway once those comments are done let's get this merged and we can finally get back to the original task of t-SNE. :)

Thanks for the hard work on this!

[rcurtin]

Suggested change

	| `bool` | **`resetPolicy`** | If true, parameters are reset before every Optimize call. | `true` |
	| `bool` | **`resetPolicy`** | If true, parameters are reset before every `Optimize()` call. | `true` |

[rcurtin]

Just for consistency, do you mind naming the member functions, like in the other optimizers' documentation?

[ranjodhsingh1729]

Hey @ranjodhsingh1729, sorry that this took me so long to get back to. I verified the implementations using the original Delta-Bar-Delta paper and the MATLAB t-SNE reference implementation. Everything looks good implementation-wise. I have a whole bunch of other comments but they should be pretty easy and quick to handle. If anything I wrote is wrong don't hesitate to call me out on it---you've spent more time in the details of these algorithms than I have. Anyway once those comments are done let's get this merged and we can finally get back to the original task of t-SNE. :)
Thanks for the hard work on this!

Hi @rcurtin, no worries - i was also tied up in academics. Thanks for your time and patient with this. It ended up taking far more effort than i anticipated, and i made mistakes, but learned a lot. I've taken care of all the comments (i think). If i missed anything or something still needs changes, let me know.

[rcurtin]

Awesome work @ranjodhsingh1729, thank you so much for working on this! I don't have any more comments before merge; thanks again for working through each of the comments that I left. Once this is merged, I'll do an ensmallen release, and then I will turn my attention (as I'm able to) to t-SNE in the mlpack repository.

[rcurtin]

Yep, that's just fine! Thanks.

[rcurtin]

Not really, just something reasonable that solves the problem. There are a few cases where the parameters look very specific, but this is because there was a behavior change for how batch sizes and step sizes were handled together; but, you will see in those cases that there are #ifdefs surrounding the legacy behavior/parameters.

[github-actions]

Second approval provided automatically after 24 hours. 👍

[rcurtin]

Hey @ranjodhsingh1729, I realized before I merged this that our CI setup had not adequately tested it with FP16 types or with Bandicoot. You can do this by installing a version of Armadillo that has FP16 support locally, and also installing Bandicoot locally, and then configuring CMake with

-DARMADILLO_INCLUDE_DIR=/path/to/arma/tmp/include/ -DARMADILLO_LIBRARY=/path/to/arma/libarmadillo.so -DBANDICOOT_INCLUDE_DIR=/path/to/coot/build/tmp/include/ -DBANDICOOT_LIBRARY=/path/to/coot/build/libbandicoot.so -DCMAKE_CXX_FLAGS="-DARMA_FORCE_USE_FP16" -DCMAKE_CXX_STANDARD=23

which is a bit of a mouthful...

Anyway, I had it set up locally so I built it all and ran the tests 1000 times with different random seeds, and debugged a few compilation failures with FP16 elements and a couple convergence issues (FP16 has more trouble converging because of its much more limited precision). I added everything here as suggestions---you can accept them, or let me know if something's wrong with them, otherwise I'll just wait a couple days then merge the suggestions, then merge the PR. Overall I am really happy that there were only a couple of changes to be made to make the optimizer work either in reduced precision or on the GPU---it means we are doing a good job of implementing the algorithms in an abstract enough way that they can run wherever! (Now, it is not true of all optimizers in ensmallen, but for DeltaBarDelta and gradient descent in general, it's actually pretty well-suited to the GPU so long as the size of coordinates is sufficiently large to get any speedup out of the GPU for the matrix multiplications and other operations... and also so long as the FunctionType being optimized runs well enough on the GPU.)

[ranjodhsingh1729]

Hi @rcurtin, Thanks for the suggestions (and their detailed descriptions). :)
It kind of slipped my mind that i needed to test with fp16 and bandicoot too. :(

I tried to tests the changes locally, but either something is wrong with my setup or maybe something is missing in suggestions. I am getting compilation errors due to type mismatches in the update rule. So if you have working changes locally, could you please just push them directly to this branch? If you have the time that is. Otherwise, I'll try to dive deeper tommorrow and see if something is wrong on my side or something needs fixing.

Thanks again!

This is where i got so far:

coot was raising error while trying to perform element-wise multiplication or subtraction between unsigned long and float matrices. After some reading of bandicoot docs and not finding anyting, I decided to just slap two more conv_to leading to this not so good code:

      const MatType signMatrix = sign(delta % deltaBar);

      const MatType increment = conv_to<MatType>::from(
          (signMatrix == +1));
      const MatType decrement = conv_to<MatType>::from(
          (signMatrix == -1));

      epsilon += conv_to<MatType>::from(increment * kappa 
          - decrement * phi % epsilon);
      epsilon.clamp(minStepSize, arma::Datum<ElemType>::inf);

After this, I got another in momentum_delta_bar_delta_update.hpp. Which i can't seem to fix yet ( i am kind of blaming bandicoot for this one :)

[rcurtin]

i am kind of blaming bandicoot for this one :)

Actually you are right to do that! What I failed to mention in the comment I posted was that the DeltaBarDelta rule actually exposed a bug in Bandicoot that I fixed here: https://gitlab.com/bandicoot-lib/bandicoot-code/-/merge_requests/192 Sorry that I forgot about that part, but based on the output you gave it looks like you debugged it or worked around it all the way regardless.

Anyway, if you want to test with "working" Bandicoot, you either have to use that particular branch or wait a day or so until I release the next version of Bandicoot with that fix included.

It's my hope to make the CI for ensmallen a little bit better so these things are automatically caught by CI instead of making developers manually check out and test with FP16 and Bandicoot, etc., (it takes a long time and is tedious!), but, not there yet.

[ranjodhsingh1729]

Thanks for letting me know.
I just had a feeling but didn't want to dive in bandicoot's codebase and confirm :).

[rcurtin]

Thanks again @ranjodhsingh1729! Happy to get this merged 🚀 I'll get a new release in progress now.