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

Fix missing square in proximal term of FedProx baseline #2210

Merged
merged 18 commits into from
Sep 19, 2023

Conversation

charlesbvll
Copy link
Member

@charlesbvll charlesbvll commented Aug 11, 2023

Issue

Description

The original FedProx paper describes the proximal term as $\frac{\mu}{2} || \omega - \omega_t ||^2$, but the current implementation sets it as $\frac{\mu}{2} || \omega_ - \omega_{t} ||$.

Related issues/PRs

N/A

Proposal

Explanation

Add torch.square to the proximal term.

Checklist

  • Implement proposed change
  • Make CI checks pass
  • Ping maintainers on Slack (channel #contributions)

Any other comments?

N/A

@jafermarq
Copy link
Contributor

I've re-run the experiments. I'll update the figure shown at the bottom of the readme

Copy link
Member

@danieljanes danieljanes left a comment

Choose a reason for hiding this comment

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

Let's rename Viz_and_plot_results.ipynb to use all lowercase characters

@charlesbvll charlesbvll marked this pull request as ready for review September 13, 2023 12:34
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.

cool

@jafermarq jafermarq enabled auto-merge (squash) September 18, 2023 17:25
@jafermarq jafermarq merged commit cbe2bc4 into main Sep 19, 2023
25 checks passed
@jafermarq jafermarq deleted the fix-fedprox-term branch September 19, 2023 17:47
tanertopal added a commit that referenced this pull request Sep 22, 2023
* 'main' of github.com:adap/flower:
  Fix default contiguous value in IidPartitioner (#2406)
  Update FDS docs index (#2337)
  Add TensorFlow integration tests with FDS (#2350)
  Add paths specification to CI triggers for FDS (#2399)
  Add FDS how-to guides (#2332)
  Add Flower Datasets tests as GitHub workflow (#2345)
  Fix the reference API documentation (#2397)
  Add FDS tutorial docs (#2375)
  Update tutorial-series-what-is-federated-learning.ipynb (#2396)
  Fix missing square in proximal term of FedProx baseline (#2210)
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