-
Notifications
You must be signed in to change notification settings - Fork 286
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
base: master
Are you sure you want to change the base?
[WIP] Implement SSL-EY #1443
Conversation
Oh wow, thanks for the PR! This looks interesting, will have a look ASAP. |
Just to make sure, is this already ready for review? |
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 |
Awesome, let use know once it is ready :) |
Btw. we can also run some benchmarks on our side if that helps :) |
Codecov ReportAttention: Patch coverage is
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. |
The failing codecov upload is unrelated to the PR :) |
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 |
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.
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.
lightly/utils/dist.py
Outdated
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.
Fixed typed errors in this module because it is used in ssley_loss
@jameschapman19 I fixed merged conflicts and some typing issues. I also added a |
No description provided.