-
Notifications
You must be signed in to change notification settings - Fork 833
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
Implement FedMedian #1461
Implement FedMedian #1461
Conversation
Thanks for the PR @edogab33 ! General code structure looks good. CI is failing because of wrongly sorted imports: https://github.com/adap/flower/actions/runs/3282834654/jobs/5406769736 The easiest way to fix this is to run |
@edogab33 let me know if we can help with making the CI checks pass. There are still a few linter warning, but they should be easy to resolve. |
I was looking for a way to run GitHub actions locally without using Docker in order to not spam useless commits in your repository but then I had to come back to my original work so I put that on hold :D I'll try to solve the issue ASAP without running the tests locally, I apologize in advance for the number of small commits. |
Thanks for being so considerate @edogab33, appreciated :) Don't worry about many small commits, we always squash commits when the PR gets merged, so it'll look like one clean commit on If we can manage to merge this PR before the end of the week, it will make it into the Flower 1.1 release. |
It seems like the test doesn't like @danieljanes Any idea on how to solve this? |
@edogab33 that's surprising, Feel free to add a |
@danieljanes yes, that's odd. However I implemented the workaround, now it's ready to be merged. As soon as possible I'll start to include all the other strategies I've implemented so far :) |
@edogab33 awesome, I look forward to these other strategies! & great, PR is green 🎉 I'm just waiting on feedback from one other reviewer. If you still up for it you could add a section to the release notes (in the |
This PR looks good. I'm just not sure about the number of tests. Apparently, it would just need the last one. @danieljanes , do we have any policy on these? |
@pedropgusmao no policy yet, I'd suggest we merge and then update if our approach to testing changes |
I did not have the time to update the changelog before the merge. Should I open a new PR for it? EDIT: Nevermind, saw you did it 👍🏼 |
@edogab33 all good, thanks for checking! |
Reference Issues/PRs
Fixes #1405.
What does this implement/fix? Explain your changes.
Implemented the aggragation function FedMedian.
Any other comments?
Let me know if everythink is ok so that I can also implement other aggregation functions. Should I also update the CODEOWNERS file?