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

Add All4One #382

Merged
merged 11 commits into from
Jan 8, 2024
Merged

Add All4One #382

merged 11 commits into from
Jan 8, 2024

Conversation

ImaGonEs
Copy link
Contributor

…tional enconding utils added. README file adapted to include All4One.

@vturrisi
Copy link
Owner

vturrisi commented Jan 4, 2024

hey @ImaGonEs thanks for contributing. We are just missing some things that need to be added before merging. Can you add the tests and documentation? You can copy-paste them from the available methods and change them accordingly. Another thing, I noticed that the config file for imagenet-100 is missing.

@ImaGonEs
Copy link
Contributor Author

ImaGonEs commented Jan 6, 2024

Hi!

Tests and documentation added (I adapted the ones you created for NNCLR/NNBYOL). ImageNet100 config file is now available also.

Tell me if anything is missing :)

@vturrisi
Copy link
Owner

vturrisi commented Jan 6, 2024

@ImaGonEs thanks for that. I took a quick look, but I'll read it more carefully later. One thing is to move the documentation of the positional_encodings to solo/utils.rst instead of having a separate file for it.

@ImaGonEs
Copy link
Contributor Author

ImaGonEs commented Jan 8, 2024

Sure. I moved it to utils.rst and added the temperature parameter to the test (I missed it on the last commit).

@vturrisi vturrisi changed the title Method, results, checkpoints and CIFAR config added for All4One, posi… Add All4One Jan 8, 2024
Copy link

codecov bot commented Jan 8, 2024

Codecov Report

Attention: 74 lines in your changes are missing coverage. Please review.

Comparison is base (5b564c2) 81.78% compared to head (bfcc95e) 81.24%.

Additional details and impacted files
Flag Coverage Δ *Carryforward flag
cpu 79.42% <72.38%> (-0.45%) ⬇️
dali 38.81% <ø> (ø) Carriedforward from 04d3f35

*This pull request uses carry forward flags. Click here to find out more.

Files Coverage Δ
solo/methods/__init__.py 100.00% <100.00%> (ø)
solo/utils/__init__.py 71.42% <ø> (ø)
solo/methods/all4one.py 95.62% <95.62%> (ø)
solo/utils/positional_encodings.py 47.69% <47.69%> (ø)

@vturrisi
Copy link
Owner

vturrisi commented Jan 8, 2024

@ImaGonEs I've fixed some minor stuff on the code that was out of standard. I'll merge the code, if there's anything else, let me know.

@vturrisi vturrisi merged commit a151225 into vturrisi:main Jan 8, 2024
9 of 13 checks passed
@ImaGonEs
Copy link
Contributor Author

Thank you for the effort! I did the code using an older version, sorry for the inconvenience.
Now that I checked it, the single thing I miss is a note in the news section (I forgot to add it in my commits). Should I add it? :)

@vturrisi
Copy link
Owner

@ImaGonEs I'll add it :)

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.

3 participants