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

PR3: Add cifar10 experiments #171

Merged
merged 18 commits into from
Oct 7, 2024
Merged

PR3: Add cifar10 experiments #171

merged 18 commits into from
Oct 7, 2024

Conversation

sanaAyrml
Copy link
Collaborator

PR Type

[Feature ]

Short Description

Cifar10 experiments with various levels of heterogeneity.

Tests Added

No tests added yet.

@sanaAyrml sanaAyrml changed the base branch from main to sa_add_deep_mmd_loss June 7, 2024 00:47
@sanaAyrml sanaAyrml requested a review from emersodb June 7, 2024 06:38
Copy link
Collaborator

@emersodb emersodb left a comment

Choose a reason for hiding this comment

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

Mostly cosmetic changes suggested throughout. The biggest set of possible changes are proposed in this PR

fl4health/utils/load_data.py Outdated Show resolved Hide resolved
fl4health/utils/load_data.py Outdated Show resolved Hide resolved
fl4health/utils/sampler.py Show resolved Hide resolved
fl4health/utils/sampler.py Outdated Show resolved Hide resolved
fl4health/utils/load_data.py Outdated Show resolved Hide resolved
research/cifar10/model.py Outdated Show resolved Hide resolved
research/cifar10/model.py Outdated Show resolved Hide resolved
research/cifar10/model.py Outdated Show resolved Hide resolved
research/cifar10/model.py Outdated Show resolved Hide resolved
sampler = DirichletLabelBasedSampler(unique_labels=list(range(10)), sample_percentage=1.0, beta=0.1)
ds = MnistDataset(data_path=Path("examples/datasets/MNIST"), train=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We're forming the MnistDataset in a few places and this test will require downloading the dataset if it doesn't exist in the folder specified. So I'm thinking we can just use a synthetic dataset for these tests. I encapsulated the possible simplification in this PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

Even though this is a comment on outdated code, we're still using the MNIST dataset here. So I think the migration to the synthetic dataset will still be useful unless you disagree 🙂

@sanaAyrml sanaAyrml changed the base branch from sa_add_deep_mmd_loss to sa_update_mkmmd_loss September 16, 2024 15:47
@sanaAyrml sanaAyrml changed the base branch from sa_update_mkmmd_loss to sa_add_deep_mmd_loss September 16, 2024 15:47
@sanaAyrml sanaAyrml changed the base branch from sa_add_deep_mmd_loss to sa_update_mkmmd_loss September 17, 2024 18:48
@sanaAyrml sanaAyrml changed the title PR4: Add cifar10 experiments PR3: Add cifar10 experiments Sep 17, 2024
Copy link
Collaborator

@emersodb emersodb left a comment

Choose a reason for hiding this comment

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

Just a few intermediate comments. I know you're still working on this one a bit 🙂

research/cifar10/ditto/client.py Show resolved Hide resolved
research/cifar10/ditto/client.py Show resolved Hide resolved
research/cifar10/ditto/client.py Outdated Show resolved Hide resolved
research/cifar10/ditto/server.py Show resolved Hide resolved
research/cifar10/ditto_mkmmd/client.py Outdated Show resolved Hide resolved
research/cifar10/evaluate_on_test.py Outdated Show resolved Hide resolved
@sanaAyrml
Copy link
Collaborator Author

Resolved all feedback and implemented four checkpointing methods for clients and two for the server. Additionally, expanded the test file to evaluate all checkpoint scenarios.

Base automatically changed from sa_update_mkmmd_loss to add_mkmmd_loss September 24, 2024 03:59
Copy link
Collaborator

@emersodb emersodb left a comment

Choose a reason for hiding this comment

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

Overall, this looks pretty good to me. However, there are a few small bugs to fix and perhaps a comment or two that would be useful.

research/cifar10/ditto/client.py Outdated Show resolved Hide resolved
research/cifar10/ditto/client.py Outdated Show resolved Hide resolved
research/cifar10/ditto_mkmmd/client.py Show resolved Hide resolved
research/cifar10/ditto_mkmmd/client.py Outdated Show resolved Hide resolved
research/cifar10/fedavg/client.py Outdated Show resolved Hide resolved
research/cifar10/evaluate_on_test.py Outdated Show resolved Hide resolved
research/cifar10/evaluate_on_test.py Outdated Show resolved Hide resolved
research/cifar10/evaluate_on_test.py Show resolved Hide resolved
research/cifar10/evaluate_on_test.py Outdated Show resolved Hide resolved
research/cifar10/evaluate_on_test.py Show resolved Hide resolved
@emersodb
Copy link
Collaborator

To other final comments:

  1. It would be good if research/cifar10 had a readme, at least to discuss how the dataset preprocessing is executed.
  2. I think we might want to implement this suggestion for the test code: PR3: Add cifar10 experiments #171 (comment) to make sure we're not repeatedly downloading mnist

Base automatically changed from add_mkmmd_loss to main September 25, 2024 17:51
@sanaAyrml
Copy link
Collaborator Author

I have made all the necessary changes and added a README to the research/cifar10 folder, detailing the required scripts for running the experiments.

@emersodb emersodb self-requested a review October 4, 2024 23:40
Copy link
Collaborator

@emersodb emersodb left a comment

Choose a reason for hiding this comment

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

Everything looks good to me once we get this smoke test fixed.

@sanaAyrml sanaAyrml merged commit 4b95db8 into main Oct 7, 2024
6 checks passed
@sanaAyrml sanaAyrml deleted the sa_add_cifar10_experiments branch October 7, 2024 16:52
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