Skip to content

Validate the optimizer betas at construction - #4310

Open
ayaangazali wants to merge 1 commit into
ml-explore:mainfrom
ayaangazali:optimizer-betas-validation
Open

Validate the optimizer betas at construction#4310
ayaangazali wants to merge 1 commit into
ml-explore:mainfrom
ayaangazali:optimizer-betas-validation

Conversation

@ayaangazali

Copy link
Copy Markdown
Contributor

None of the four optimizers that take betas check 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 of 0.1:

Adam(betas=[0.9, 1.0])     ->  [-1314409.0, -1314408.0, -1314407.0]
Adam(betas=[1.5, 0.999])   ->  [25.48, 26.48, 27.48]      # moves away from the minimum
Adam(betas=[-0.5, 0.999])  ->  [-10.42, -9.42, -8.42]

beta == 1.0 is the worst of them: the moving average never takes in a new gradient, and with bias_correction=True the correction divides by 1 - beta**t, which is exactly zero.

This is a gap rather than a policy, since eps is already guarded in four places with the same shape of check:

if self.alpha < 0.0:
    raise ValueError(f"RMSprop alpha should be >=0, {self.alpha} was provided instead")
if self.eps <= 0.0:
    raise ValueError(f"RMSprop epsilon should be >0, {self.eps} was provided instead")

Adagrad, AdaDelta and Adamax do the same for their own eps and rho. betas are the one hyperparameter nothing validates.

pytorch rejects the same values at construction, with ValueError: Invalid beta parameter at index 0: 1.5.

This validates betas in Adam.__init__, which also covers AdamW and Adamax since both route through it, and in Lion.__init__, which keeps its own. Message follows the existing wording:

ValueError: Adam beta2 should be in [0, 1), 1.0 was provided instead

I deliberately left eps alone in Adam. Adamax already validates its own eps as >= 0 while RMSprop and the others require > 0, so adding a check in the shared Adam.__init__ would have quietly tightened Adamax from one rule to the other. That is a separate decision and not this bug.

Checked that the allowed values still construct, including the 0.0 endpoint 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.py and test_array.py pass. 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.

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.
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.

1 participant