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

[WIP] Implement SSL-EY #1443

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

jameschapman19
Copy link

No description provided.

@guarin
Copy link
Contributor

guarin commented Dec 6, 2023

Oh wow, thanks for the PR! This looks interesting, will have a look ASAP.

@guarin
Copy link
Contributor

guarin commented Dec 6, 2023

Just to make sure, is this already ready for review?

@jameschapman19
Copy link
Author

jameschapman19 commented Dec 6, 2023

Hi,

Still WIP but close and will let you know.

Need to run the test suite locally. Will also run all the benchmarks I am able to (I have written the scripts in the right format though). I put a PR in as I thought it might run the CI and help me fix things up before review.

In many ways the pitch of the original work is VICReg without any hyperparameters in the objective so it plugs in quite easily.

James

@jameschapman19 jameschapman19 changed the title Implement SSL-EY [WIP] Implement SSL-EY Dec 6, 2023
@guarin
Copy link
Contributor

guarin commented Dec 6, 2023

Awesome, let use know once it is ready :)

@guarin
Copy link
Contributor

guarin commented Dec 7, 2023

Btw. we can also run some benchmarks on our side if that helps :)

Copy link

codecov bot commented Dec 11, 2023

Codecov Report

Attention: Patch coverage is 91.52542% with 5 lines in your changes missing coverage. Please review.

Project coverage is 85.58%. Comparing base (534cdf4) to head (f88efd0).
Report is 110 commits behind head on master.

Files Patch % Lines
lightly/loss/ssley_loss.py 87.09% 4 Missing ⚠️
lightly/utils/dist.py 93.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1443      +/-   ##
==========================================
+ Coverage   85.53%   85.58%   +0.05%     
==========================================
  Files         135      137       +2     
  Lines        5655     5703      +48     
==========================================
+ Hits         4837     4881      +44     
- Misses        818      822       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@guarin
Copy link
Contributor

guarin commented Dec 11, 2023

The failing codecov upload is unrelated to the PR :)
Could you run make format to apply the autoformatting? See https://github.com/lightly-ai/lightly?tab=readme-ov-file#development for more info.

from lightly.loss.ssley_loss import SSLEYLoss
from lightly.models.modules.heads import VICRegProjectionHead
from lightly.models.utils import get_weight_decay_parameters
from lightly.transforms.ssley_transform import VICRegTransform
Copy link
Contributor

Choose a reason for hiding this comment

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

This import doesn't work as ssley_transform doesn't exist yet. I think it would make sense to add a ssley_transform.py module and create a SSLEYTransform which inherits from VICRegTransform. That way one doesn't have to remember that ssley uses the vicreg transform.

This was referenced Jan 2, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed typed errors in this module because it is used in ssley_loss

@guarin
Copy link
Contributor

guarin commented Jan 5, 2024

@jameschapman19 I fixed merged conflicts and some typing issues. I also added a SSLEYProjectionHead changed the examples to use the SSLEYTransform. I'll now try training the model on imagenet and then try to get this PR merged :)

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.

2 participants