Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #7 #8

Merged
merged 4 commits into from Nov 11, 2021
Merged

Fix #7 #8

merged 4 commits into from Nov 11, 2021

Conversation

rainwoodman
Copy link
Contributor

Fix #7.

The SAM training accuracy is near 82.5%, increased from the quoted numbers on the home page.

The non-SAM training appears to terminated early at 5m30, ~ 70 epochs, and at a lower accuracy than the home page number.

@rainwoodman
Copy link
Contributor Author

I am not sure if this ipynb file is clean -- I see some references to my own github repo.

@sayakpaul
Copy link
Owner

@rainwoodman the notebook is great. Just a few minor suggestions:

  • In the cell that implements SAM class I guess we can skip the TensorFlow import. Also, I don't think we need to disable eager execution for the current setup. My understanding is that when train_step() is overridden and the underlying model is compiled it is going to run in graph mode already. However, please correct me if I am missing out on something here.
  • You are welcome to edit the README numbers with the latest ones. This will give the users more confidence in using SAM I hope.

@rainwoodman
Copy link
Contributor Author

You are right I forgot to remove the lines. Those were for debugging, when I checked that the before this change indeed we used to have gradients == sam_gradients.

@rainwoodman
Copy link
Contributor Author

I removed the lines, and updated README. Please take a look, thanks!

Copy link
Owner

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rainwoodman thank you for the changes. Just one more minor comment.

@@ -27,8 +27,8 @@ SAM has only one hyperparameter namely `rho` that controls the neighborhood of t
| Without SAM | 0.575114 | 82.9 |
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you also want to update this table with the final scores?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes It was updated in the previous commit. ;)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. Anyway the current README should reflect the changes you made. Thank you so much.

@sayakpaul sayakpaul merged commit 8b08871 into sayakpaul:main Nov 11, 2021
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.

Potential bug
2 participants