-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Adding an example for Supervised Contrastive Learning #283
Conversation
@fchollet - kindly review the PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR. Please only include the Python file in the PR for now.
Done - Only the Python file is included in the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update!
@fchollet - Have addressed your comments. Kindly review my revisions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the update!
@fchollet kindly find my revisions. Cheers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Just a few minor comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Looks good, please use preprocessing layers and address the remaining comments.
@fchollet - I think I have addressed all your comments, including:
Please let me know if there is anything else need to be modified. |
I have simplified the models by removing the BatchNorm layers as they don't improve the results |
The code I'm seeing still uses ImageDataGenerator; did you push the changes to the PR? |
@fchollet - Sorry for the confusion. I've just pushed the changes and they should be now part of the PR |
Also made some formatting updates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Please address the remaining nits and add the autogenerated .md
and .ipynb
files (you can generate them with the python autogen.py add_example command
)
Nits addressed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, please add the generated files to the PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ksalama I'm a tf.keras user here and just came across the PR. This example is awesome.
Can you please check and change RestNet
to ResNet
(assuming you're discussing residual networks like in https://arxiv.org/abs/1512.03385).
architecture, like ResNet, and multi-class problems with many labels.
Can I also suggest to improve the introduction just a little bit to help the readers understand what they're about to learn? You could, for example, write about 1) what the user will learn about - in a bit more detail (can be summarized from the paper on arXiv (with the link included) before going into the two-phase structure that you already mentioned; 2) what the user will do - it's a classification task for X using Y dataset. Both of these could be just 1-2 short sentences.
Also, I'd advise to copy-paste the Markdown text into Google Docs just to check that all grammar and spelling is 👍 - but I've not spotted anything out of the ordinary here.
Anyway, these are just suggestions from experience. I really enjoyed the paper, thanks for sharing the work!
outperformed the conventional technique in terms of the test accuracy. Note that | ||
the same training budget (i.e., number of epochs) was given to each technique. | ||
Supervised contrastive learning pays off when the encoder involves a complex | ||
architecture, like RestNet, and multi-class problems with many labels. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please change RestNet
to ResNet
(assuming you're discussing residual networks like in https://arxiv.org/abs/1512.03385)?
architecture, like RestNet, and multi-class problems with many labels. | |
architecture, like ResNet, and multi-class problems with many labels. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @8bitmp3 for spotting this. I have applied the fixes 👍
@fchollet I have added the generated .md and .ipynb files to the PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, thank you for the great contribution! I'll take it from there 👍
Supervised Contrastive Learning: https://arxiv.org/abs/2004.11362