chore: changed the assertions to make sure the num_updates is a multiple of num_evaluations

[Louay-Ben-nessir]

What?

Changed the assertions to make sure the num_updates is a multiple of num_evaluations.

Why?

Only num_evaluation * num_updates_per_eval are ran while training which can lead to some missed updates if the num_updates is not a multiple of num_evaluations.

How?

changed the assertions.

[OmaymaMahjoub]

Thanks @Louay-Ben-nessir, can you as well check if the other systems suffer from the same problem or not 🙏

[sash-a]

A suggestion here that would remove the need for the assert and make mava easier to configure is to change the variable to evaluation frequency and then store num_evaluations in the config as config.arch.num_evals = config.system.num_updates // config.arch.eval_freq so that we still have that info when logging

[Louay-Ben-nessir]

Thanks @Louay-Ben-nessir, can you as well check if the other systems suffer from the same problem or not 🙏

I think this issue is exclusive to ppo systems

A suggestion here that would remove the need for the assert and make mava easier to configure is to change the variable to evaluation frequency and then store num_evaluations in the config as config.arch.num_evals = config.system.num_updates // config.arch.eval_freq so that we still have that info when logging

This a huge improvement over the current implementation but it's still not exact in some cases. losing some updates is worth it for the flexibility tho so I'll change it.

[sash-a]

Ah right we could lose some updates. @RuanJohn has had an issue with this in the past, so maybe a jnp.ceil is needed here, just double check with him

[RuanJohn]

The hard assert here is a bit too strict in my opinion.
Something we could do is to make a warning that says the number of timesteps someone is assuming their experiment will run for might not happen and then give the total number of timesteps that will run.