Implement FedMedian

[edogab33]

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?

[danieljanes]

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 ./dev/format.sh

[danieljanes]

@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.

[edogab33]

@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.

[danieljanes]

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 main.

If we can manage to merge this PR before the end of the week, it will make it into the Flower 1.1 release.

[edogab33]

It seems like the test doesn't like np.median() at all.

@danieljanes Any idea on how to solve this?

[danieljanes]

@edogab33 that's surprising, np.median should have type annotations as of NumPy 1.22: numpy/numpy#20034

Feel free to add a # type: ignore at the end of the line that calls np.median, we'll take care of this after increasing the minimum NumPy version to 1.22.

[edogab33]

@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 :)

[danieljanes]

@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 Unreleased section): https://github.com/adap/flower/blob/main/doc/source/changelog.md (ideally a short bold headline + PR ref followed by a description of one or more sentences)

[pedropgusmao]

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?

[danieljanes]

@pedropgusmao no policy yet, I'd suggest we merge and then update if our approach to testing changes

[edogab33]

@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 Unreleased section): https://github.com/adap/flower/blob/main/doc/source/changelog.md (ideally a short bold headline + PR ref followed by a description of one or more sentences)

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 👍🏼

[danieljanes]

@edogab33 all good, thanks for checking!