FedAvgM expecting initial_parameters as Optional

[gubertoli]

Describe the bug

The current implementation of FedAvgM strategy considers the initial_parameters as Optional in the class init method (here):

initial_parameters: Optional[Parameters] = None

However, the FedAvgM strategy requires the initial_parameters (here):

            assert (
                self.initial_parameters is not None
            ), "When using server-side optimization, model needs to be initialized."
            initial_weights = parameters_to_ndarrays(self.initial_parameters)

Steps/Code to Reproduce

strategy = fl.server.strategy.FedAvgM()

fl.simulation.start_simulation(
    client_fn=client_fn,
    num_clients=NUM_CLIENTS,
    config=fl.server.ServerConfig(num_rounds=5),
    strategy=strategy,
)

Expected Results

To run the a simulation with vanilla FedAvgM strategy without initial_parameters.

Actual Results

Assert exception: "When using server-side optimization, model needs to be initialized."

[DanielCardeal]

As far as I can tell, initial_parameters are required when using server side optimization in the FedAvgM strategy, as indicated by this if statement above the assertion clause.

Server side optimization is enabled whenever server_learning_rate or server_momentum constructor parameters are set to values different from the defaults (here). As such, calling the FedAvgM constructor without any parameters works fine, while calling it with server_learning_rate = 0.9, for example, don't.

Considering all of that, it could be nice to add this information to the constructors' documentation, as to avoid confusion in the future.

[achiverram28]

I guess the assert is an explanatory point and agree with @DanielCardeal where the user can understand that if there is a server side optimisation , the initial_parameters should be done , but , for more clarity we can add it in the docstrings of the strategy class

[gubertoli]

Isn't FedAvgM about server side optimization with momentum?

[DanielCardeal]

Yes, @gubertoli.

It also seems a bit counterintuitive to allow users to initialize the strategy without initial_parameters since, in such cases, both FedAvg and FedAvgM aggregate the results in the exact same way.

I propose the changes in #2416 as a possible fix.