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

Fedmix #2213

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from
Draft

Fedmix #2213

wants to merge 23 commits into from

Conversation

DevPranjal
Copy link

Add FedMix baseline

Fixes #2051

@jafermarq

Checklist

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

@DevPranjal
Copy link
Author

DevPranjal commented Aug 14, 2023

Hey @jafermarq. Here is the first working version of the FedMix baseline. I ran an experiment for CIFAR10 using the parameters mentioned in the paper, and the results were different from what were mentioned in the paper. I will look for any implementation detail that must have been missed. Best test accuracy obtained after 500 rounds is 65% currently (should have been ~80%)
centralized_metrics

@jafermarq jafermarq added the summer-of-reproducibility About a baseline for Summer of Reproducibility label Aug 15, 2023
@jafermarq
Copy link
Contributor

@DevPranjal Thanks for the update! It seems your Fedmix implementation is going in the right direction :)

@jafermarq
Copy link
Contributor

Hey @DevPranjal , any updates for your baseline ? The Summer of Reproducibility ends in 10 days.

@DevPranjal
Copy link
Author

Hello @jafermarq! Really sorry for the late reply. I got caught up in a little academic workload. I have been trying out a lot in reproducing the paper recently, but haven't been able to reproduce the author's results. Here are a few updates:

  • I started off with my own implementation of the paper, and tested the code on CIFAR10, the results of which had been attached above in my comment. Continuing from here, I also obtained the results below, which clearly shows the trend mentioned in the paper, but did not reproduce the numbers:
    fedmix_vs_fedavg

  • I then suspected a probable error in data partitioning, for which I contacted the authors but did not receive any reply. I also tried other (unofficial) open-source FedMix code available on GitHub with no success.

  • Yesterday, I also tested the code on FEMNIST data (partitioning will not an issue, since it is pretty standard) and I still could not reproduce the results. Here are the results that I obtained:

Strategy Reproduced Test Accuracy Reported Test Accuracy
FedAvg 80.6% 85.3%
NaiveMix 78.5% 85.9%
FedMix 78.9% 86.5%

Working on reproducing the reported results!

@jafermarq
Copy link
Contributor

@DevPranjal , thanks for the update! I'll look into your baseline soon (likely today). I will leave some feedback comments for you and maybe I can spot where the issue is.

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 @DevPranjal,

I have done a first pass through your baseline and left some comments and suggestions. You can apply the changes directly or add a comment if something is not clear. Please flag those you change as "resolved". In addition to that:

  • Please remove the EXTENDED_README.md from your directory as it was only intended to guide you through the initial steps in creating a baseline.
  • If you make some plots with your results and you include them in the README.md, please keep those in a new directory called _static (which should be placed in the same directory as the readme).

After adding some packages to the pyproject.toml and made some of the changes i suggested (i.e. num_cpus passed to start_simulation) i was able to launch a generic experiment like this:

python -m fedmix.main 

Let me know if this is the command that would generate the results you showed above.

baselines/fedmix/pyproject.toml Outdated Show resolved Hide resolved
baselines/fedmix/pyproject.toml Outdated Show resolved Hide resolved
baselines/fedmix/pyproject.toml Show resolved Hide resolved
baselines/fedmix/pyproject.toml Outdated Show resolved Hide resolved
baselines/fedmix/README.md Show resolved Hide resolved
baselines/fedmix/fedmix/conf/cifar10.yaml Outdated Show resolved Hide resolved
baselines/fedmix/fedmix/conf/cifar10.yaml Outdated Show resolved Hide resolved
baselines/fedmix/fedmix/main.py Outdated Show resolved Hide resolved
baselines/fedmix/fedmix/client.py Show resolved Hide resolved
@jafermarq
Copy link
Contributor

@DevPranjal , please let me know when you would like me to take another look to your baseline. Happy to discuss further the points above.

@DevPranjal
Copy link
Author

Thanks for the review @jafermarq.

Let me know if this is the command that would generate the results you showed above.

Yes, I used this to get the results above. I have made the changes requested in the comments on my remote machine, and will push them right away.

@DevPranjal
Copy link
Author

Hello @jafermarq. I have added the code for FEMNIST data as well, and changed the config structure to make it more user-friendly.

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 @DevPranjal ,

I'm going through your baseline again. Please find some comments below (mostly minor). I think the main one is to remove the code you borrowed from LEAF and instead tell users to clone the repo themselves. You can add some instructions for them to follow.

baselines/fedmix/README.md Show resolved Hide resolved
baselines/fedmix/README.md Show resolved Hide resolved
baselines/fedmix/fedmix/conf/config.yaml Outdated Show resolved Hide resolved
baselines/fedmix/fedmix/femnist/README.md Outdated Show resolved Hide resolved
baselines/fedmix/pyproject.toml Show resolved Hide resolved
baselines/fedmix/pyproject.toml 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.

Some further comment after looking closely into your dataset creation process.

baselines/fedmix/fedmix/dataset_preparation.py Outdated Show resolved Hide resolved
baselines/fedmix/README.md Outdated Show resolved Hide resolved
DevPranjal and others added 3 commits October 16, 2023 22:58
Co-authored-by: Javier <jafermarq@users.noreply.github.com>
Co-authored-by: Javier <jafermarq@users.noreply.github.com>
@DevPranjal
Copy link
Author

@jafermarq have made some changes. Please have a look. Also attaching current progress for the record
table

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.

FedMix
2 participants