google-research / google-research/sam

L2 regulariser and SAM

Open
#24 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
646
Forks
81
PR merge metrics
No merged PRs in 30d

Description

It seems to me that there might be a mistake in the way the noised state is computed in the current implementation. Specifically

[sam](https://github.com/google-research/sam/tree/main)/[sam_jax](https://github.com/google-research/sam/tree/main/sam_jax)/[training_utils](https://github.com/google-research/sam/tree/main/sam_jax/training_utils/flax_training.py)

in line 537, `forward_and_loss` which includes the l2 regularization is used to compute `grad` this is then used in line 546 as input to `dual_vector(grad)`.

I think this is not exactly correct given the original SAM paper. The state shouldn't be noised for the l2 regularization as it is now, but only for the cross-entropy loss. A separate gradient for the clean state should be computed for the l2 regularization and summed with the SAM gradient.

Is there something that I'm missing?

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.