Fixed EMAHandler warm-up behaviour

[sandylaker]

Fixes #2294

Description:

• Fixed EMAHandler warm-up behaviour

Check list:

• New tests are added (if a new feature is added)
• New doc strings: description and/or example code are in RST format
• Documentation is updated (if required)

[vfdev-5]

Thanks for the PR @sandylaker !
Unfortunately, as we already released a stable version with the args you are removing here, we can not just remove them without deprecation cycle. Let's do the following, we keep the docstring as it was and add deprecation message suggesting to use StateParamScheduler and that we will remove these args in future releases.
In the code we can first fix #2294 issue and also show deprecation warning if any of these params is used.

[sandylaker]

@vfdev-5 Thanks for pointing out that.

1. Do we have a specific class or function/decorator for the deprecation warning? Or should I adopt the Python's built-in DeprecationWarning?
2. Besides warning, should we keep using the internal function for updating the momentum? I.e., we just throw a warning when the handler performs warm-up. But in this case the StateParamScheduler still cannot control the momentum.

[vfdev-5]

Do we have a specific class or function/decorator for the deprecation warning? Or should I adopt the Python's built-in DeprecationWarning?

@sandylaker I think we can do as we are doing for seed arg in Engine.run,

ignite/ignite/engine/engine.py

Line 598 in 8a1de70

seed: Optional[int] = None,

Besides warning, should we keep using the internal function for updating the momentum? I.e., we just throw a warning when the handler performs warm-up. But in this case the StateParamScheduler still cannot control the momentum.

Sorry, I didn't quite follow everything in the code for StateParamScheduler, but if it is either one or another, then let's again do as for seed <=> if user uses it the warning is shown but no seed is set = nothing done.

cc @fco-dv

[sandylaker]

@vfdev-5 So that means: we encourage the users to use StateParamScheduler. If they create a handler like this

ema_handler = EMAHandler(momentum=0.2, momentum_warmup=0.02, warmup_iters=100)

Then,

1. a warning message is shown and saying that momentum_warmup and warmup_iters will be deprecated in the future.
2. The warm-up args have no actual effect, no warmup-up will be performed. I.e., the momentum will be constant, unless users use StateParamScheduler to control it. This also means that _update_ema_momentum and _get_momentum can be removed, we just read the momentum from Engine.state.
3. We keep the current docstring and the args.

[vfdev-5]

@sandylaker yes, I was thinking about something like that. Do you think this is too restrictive ? In terms of API, I agree that it would require 2-3 additional lines with a state param scheduler in order to have the same behaviour. On the other hand we can keep both things: momentum_warmup, warmup_iters args and usage of state param handler and clearly say in docs for EMAHandler that if momentum_warmup, warmup_iters args are specified then they will overwrite any other scheduling (via state param scheduler or just a manual control).
What do you think or any other suggestions ?

[sandylaker]

@vfdev-5 I think it is better to do like this:

1. a warning message is shown and saying that momentum_warmup and warmup_iters will be deprecated in the future.
2. The warm-up args have no actual effect, no warmup-up will be performed. I.e., the momentum will be constant, unless users use StateParamScheduler to control it. This also means that _update_ema_momentum and _get_momentum can be removed, we just read the momentum from Engine.state.
3. We keep the current docstring and the args.

Because some users hope to schedule the momentum with a custom schedule. So we can provide this feature in the next version.
At the same time, we will warn the users who is using the current version that they the warmup function will be deprecated.
Users only need to update their code when they have set the warmup args.

So I think doing this will not break the BC.

[vfdev-5]

@sandylaker sorry for delay, OK, I agree with you and let's make it as you are suggesting.

[vfdev-5]

Thanks for the updates @sandylaker (and sorry for delay)
I left few other comments...

It would be nice to provide an explicit example in the docs on how to do the warm-up. Further more I wonder how could we simplify more explicit state param scheduler creation and make it a one-liner... @fco-dv any ideas ?

[vfdev-5]

Shouldn't we make at least 10 iters to properly check the scheduler ?

[vfdev-5]

Same here, IMO

[vfdev-5]

Thanks a lot for the updates @sandylaker !
I'll update the tests and can merge the PR then