Repository navigation
Change initial eigen values in KLShampoo to 1 - #310
Conversation
Signed-off-by: Hao Wu <skyw@nvidia.com>
 [Medium impact] The PR appears safe to merge; the previously noted test gap is non-blocking. Findings
SummaryKL-Shampoo now starts both eigenvalue vectors at one instead of zero.
Reviews (2) · Last reviewed commit: "grammar fix" · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "eigvals_L": torch.ones(m, device=device), | ||
| "eigvals_R": torch.ones(n, device=device), |
There was a problem hiding this comment.
The tests do not protect the behavior changed here. test_init_state_layout checks eigenvalue shapes but not their values. test_update_kronecker_factors_matches_legacy checks only a later update using the same helper. Returning to zero initialization could therefore pass both tests.
Add a first-step test with a rectangular gradient G, shampoo_beta=0, and small positive eps. Check that init_step produces L = G @ G.T / n and R = G.T @ G / m.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Signed-off-by: Hao Wu <skyw@nvidia.com>
|
/ok to test ac6a500 |
@skyw, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test ac6a500 |
@skyw, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test 070a043 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This branch was successfully deployed
1 participant
No description provided.