Repository navigation
GradSNR: drop the bias correction from ratio() - #28
Open
10xCrazyHorse wants to merge 1 commit into
Open
10xCrazyHorse wants to merge 1 commit into
10xCrazyHorse wants to merge 1 commit into
Conversation
Both averages start at the first reading, not at zero, so dividing by 1 - beta^n only inflated the ratio: 6.7x at the eighth reading, and a constant gradient read above one. Reported only, nothing steers on it.
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.
GradSNR.ratio()divides both running averages by1 - beta^n, the Adam-style bias correction. That correction is for averages that start at zero. Here both start at the first reading (self.m = flat.clone(), andself.sq = swhenn == 0), so their weights already sum to one and neither is biased toward zero. Sincemis squared in the numerator, the correction leaves the ratio multiplied by1 / (1 - beta^n): 6.7x at the eighth reading, where it is first reported.Readings at
beta = 0.98, 256 elements, before and after:randn), beforeSo for the first 50 to 100 steps of a run the
signalvalue in thereadlog sits above its documented 0..1 range: pure noise reads 4.9 on the first report, and a constant gradient only comes down to 1 asbeta^ndecays. In steady state (a few hundred steps) the two agree, and pure noise settles near(1 - beta) / (1 + beta), about 0.01, either way.The impact is diagnostic only.
ratio()feeds thesignalfield of the progress line intrain.py, and nothing steers on it (the class docstring says so), so training is unchanged; only the early readings were wrong.The fix removes the correction and returns
||m||^2 / sqdirectly. One thing it does not change: with only 8 readings the first gradient still carries about 0.87 of the weight, so pure noise reads around 0.7 at the first report. That comes from starting the averages at the first reading, not from this bug, so I left then < 8warm-up as it is.Relation to #19 / #20
Independent of both. #20 changes
observe()(what goes into the averages, for the crash in #19); this changesratio()(how they are read). The hunks don't overlap: this branch merges with #20's head (cb36633) without conflicts in either order, and the merged result passes this test and a missing-gradient test. The new test uses a single parameter that always has a gradient, so it doesn't depend on #20.Test plan
python -m pytest -q tests/test_gradsnr.py: fails on main (constant gradient reads 6.70 at the 8th reading; noise reads 4.87 after 8), passes with this fix (2 tests, CPU, under a second)Two files:
minagi/optim.py(+5/-2) and the newtests/test_gradsnr.py(+25).