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 FedVSSL baseline #2412

Merged
merged 135 commits into from
Dec 18, 2023
Merged

Add FedVSSL baseline #2412

merged 135 commits into from
Dec 18, 2023

Conversation

yasar-rehman
Copy link
Contributor

@yasar-rehman yasar-rehman commented Sep 22, 2023

Issue

#2058

Description

Related issues/PRs

Proposal

Explanation

Checklist

  • Implement proposed change
  • Write tests
  • Update documentation
  • Update changelog
  • Make CI checks pass
  • Ping maintainers on Slack (channel #contributions)

Any other comments?

Copy link
Contributor

@jafermarq jafermarq left a comment

Choose a reason for hiding this comment

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

Hi @yasar-rehman, @yan-gao-GY

This is looking good. I was able to run the experiments fine. Interestingly i got much better results for UCF-101 results when pretraining on UCF-101 than those in the table...

Could you take some time to address the comments below?

baselines/fedvssl/README.md Outdated Show resolved Hide resolved
baselines/fedvssl/README.md Outdated Show resolved Hide resolved
baselines/fedvssl/README.md Outdated Show resolved Hide resolved
baselines/fedvssl/fedvssl/client.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jafermarq jafermarq left a comment

Choose a reason for hiding this comment

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

Hi @yasar-rehman , @yan-gao-GY ,

This baseline can be merged 🥳 . But first, could you look at the comment i left below? Ping me when ready.

In the Expected Results section it says "takes a considerable amount of time without access to server-grade hardware (~5 days with a RTX 3090)". But i thought you'd need several RTX 3090? is it 6 of them as stated earlier in the README.md?

@yan-gao-GY
Copy link
Contributor

yan-gao-GY commented Dec 15, 2023

@jafermarq thanks for your recent fix! It would take ~1 day if using 6 RTX 3090. Let's change the statement in the Expected Results section to keep consistent?

@jafermarq jafermarq changed the title FedVSSL Add FedVSSL baseline Dec 15, 2023
Copy link
Contributor

@jafermarq jafermarq left a comment

Choose a reason for hiding this comment

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

@yan-gao-GY , @yasar-rehman ,

Your FedVSSL baseline is ready to be merged. Many thanks for thanks for the hard work !

@jafermarq jafermarq enabled auto-merge (squash) December 18, 2023 21:59
@jafermarq jafermarq merged commit 625d1d2 into adap:main Dec 18, 2023
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
summer-of-reproducibility About a baseline for Summer of Reproducibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants