Validate the optimizer betas at construction - #4310
Open
ayaangazali wants to merge 1 commit into
Open
Conversation
An exponential moving average needs 0 <= beta < 1, but none of the four optimizers taking betas checked them, so a beta of 1.0 or 1.5 quietly diverged instead of erroring. eps is already validated in RMSprop, Adagrad, AdaDelta and Adamax, betas were the gap.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
None of the four optimizers that take
betascheck them, so a value outside[0, 1)is accepted and the run quietly diverges.Five steps on a single parameter starting at
[1, 2, 3]with a constant gradient of0.1:beta == 1.0is the worst of them: the moving average never takes in a new gradient, and withbias_correction=Truethe correction divides by1 - beta**t, which is exactly zero.This is a gap rather than a policy, since
epsis already guarded in four places with the same shape of check:Adagrad,AdaDeltaandAdamaxdo the same for their ownepsandrho.betasare the one hyperparameter nothing validates.pytorch rejects the same values at construction, with
ValueError: Invalid beta parameter at index 0: 1.5.This validates
betasinAdam.__init__, which also coversAdamWandAdamaxsince both route through it, and inLion.__init__, which keeps its own. Message follows the existing wording:I deliberately left
epsalone inAdam.Adamaxalready validates its ownepsas>= 0whileRMSpropand the others require> 0, so adding a check in the sharedAdam.__init__would have quietly tightenedAdamaxfrom one rule to the other. That is a separate decision and not this bug.Checked that the allowed values still construct, including the
0.0endpoint and values very close to 1:[0.0, 0.0],[0.9, 0.999],[0.999, 0.9999]all fine, for all four optimizers.test_optimizers.py,test_nn.py,test_losses.py,test_compile.pyandtest_array.pypass. The added test fails on main.I am a freshman learning this codebase and I had Claude Code alongside me. Every number above is from a run on this machine.