Skip to content

Fix an off by one issue affecting number of iterations in multiple optimizers. - #443

Merged
conradsnicta merged 6 commits into
mlpack:masterfrom
ranjodhsingh1729:gradient-descent-offbyone
Nov 11, 2025
Merged

Fix an off by one issue affecting number of iterations in multiple optimizers.#443
conradsnicta merged 6 commits into
mlpack:masterfrom
ranjodhsingh1729:gradient-descent-offbyone

Conversation

@ranjodhsingh1729

@ranjodhsingh1729 ranjodhsingh1729 commented Oct 19, 2025

Copy link
Copy Markdown
Contributor

Description

This PR fixes the following issue in multiple optimizers:

Example

If maxIterations is non-zero it will perform one less iteration.

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

@rcurtin rcurtin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks; this is a really minor fix but I definitely don't mind adding it. There is a consistency issue worth thinking about here: there are a number of optimizers that use code exactly like this (before your fix), and there are also some that don't:

$ grep -r 'for (size_t i = 1' ~/src/ensmallen/include/
/home/ryan/src/ensmallen/include/ensmallen_bits/gradient_descent/gradient_descent_impl.hpp:  for (size_t i = 1; i != maxIterations && !terminate; ++i)
/home/ryan/src/ensmallen/include/ensmallen_bits/fw/frank_wolfe_impl.hpp:  for (size_t i = 1; i != maxIterations && !terminate; ++i)
/home/ryan/src/ensmallen/include/ensmallen_bits/fw/constr_structure_group.hpp:    for (size_t i = 1; i <= nGroups; ++i)
/home/ryan/src/ensmallen/include/ensmallen_bits/fista/fista_impl.hpp:  for (size_t i = 1; i != maxIterations && !terminate; ++i)
/home/ryan/src/ensmallen/include/ensmallen_bits/parallel_sgd/parallel_sgd_impl.hpp:  for (size_t i = 1; i != maxIterations && !terminate; ++i)
/home/ryan/src/ensmallen/include/ensmallen_bits/nsga2/nsga2_impl.hpp:    for (size_t i = 1; i < fSize - 1; i++)
/home/ryan/src/ensmallen/include/ensmallen_bits/iqn/iqn_impl.hpp:  for (size_t i = 1; i != maxIterations && !terminate; ++i)
/home/ryan/src/ensmallen/include/ensmallen_bits/cne/cne_impl.hpp:  for (size_t i = 1; i < populationSize; i++)
/home/ryan/src/ensmallen/include/ensmallen_bits/cmaes/active_cmaes_impl.hpp:  for (size_t i = 1; (i != maxIterations) && !terminate; ++i)
/home/ryan/src/ensmallen/include/ensmallen_bits/cmaes/cmaes_impl.hpp:  for (size_t i = 1; (i != maxIterations) && !terminate; ++i)
/home/ryan/src/ensmallen/include/ensmallen_bits/fasta/fasta_impl.hpp:  for (size_t i = 1; i != maxIterations && !terminate; ++i)
/home/ryan/src/ensmallen/include/ensmallen_bits/cd/cd_impl.hpp:  for (size_t i = 1; i != maxIterations && !terminate; ++i)
/home/ryan/src/ensmallen/include/ensmallen_bits/fbs/fbs_impl.hpp:  for (size_t i = 1; i != maxIterations && !terminate; ++i)

So it would make sense to fix all of them, but also, there is the inconsistency of iteration number: this gradient descent optimizer will print "iteration 1" on the first iteration, but, e.g., BigBatchSGD will print "iteration 0" on its first iteration. I think it would be easiest to just set the first iteration to 0 instead of 1.

Anyway, up to you whether you want to fix just this one or the other ones I grepped for too. Either way is an improvement. 👍

Comment thread include/ensmallen_bits/gradient_descent/gradient_descent_impl.hpp Outdated
Comment thread include/ensmallen_bits/gradient_descent/gradient_descent_impl.hpp Outdated
@ranjodhsingh1729 ranjodhsingh1729 changed the title Fix GradientDescent iterations off by one issue Fix an off by one issue affecting number of iterations in multiple optimizers. Oct 20, 2025
@ranjodhsingh1729

Copy link
Copy Markdown
Contributor Author

Anyway, up to you whether you want to fix just this one or the other ones I grepped for too. Either way is an improvement.

No issue! I'll add the others.

@rcurtin rcurtin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix. Do you want to add something to the changelog? Otherwise I will do it on merge. All my suggestions are just tiny style fixes.

Comment thread include/ensmallen_bits/cd/cd_impl.hpp Outdated
Comment thread include/ensmallen_bits/cmaes/active_cmaes_impl.hpp Outdated
Comment thread include/ensmallen_bits/cmaes/cmaes_impl.hpp Outdated
Comment thread include/ensmallen_bits/iqn/iqn_impl.hpp Outdated
Comment thread include/ensmallen_bits/cd/cd_impl.hpp Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second approval provided automatically after 24 hours. 👍

ranjodhsingh1729 and others added 2 commits October 31, 2025 13:56
Co-authored-by: Ryan Curtin <ryan@ratml.org>
Comment thread HISTORY.md Outdated
@conradsnicta
conradsnicta merged commit b4b403e into mlpack:master Nov 11, 2025
1 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants