Working ParameterScheduler

[danieltudosiu]

fixes #1552

Description

Added a new ParameterScheduler handler and the required tests.

Status

Ready

Types of changes

• Non-breaking change (fix or new feature that would not break existing functionality).
• New tests added to cover the changes.
• In-line docstrings updated.

[danieltudosiu]

/black

[wyli]

thanks @danieltudosiu, could you follow this PR #1940 (it's also about new handler) to update the docs/ and min_tests.py

[danieltudosiu]

@wyli Just to be sure that I do what you want me to do.

You want me to reference the inline documentation that is written in the class to create the docs by adding it to the docs/source/handlers.rst?

And you want to exclude the tests that I written from being run by min_tests.py?

Cheers!

[wyli]

exactly, thanks!

[danieltudosiu]

@wyli could you please help me with this:

In the flake8 test I get the following error:

monai/handlers/parameter_scheduler.py:52:43: error: Argument 2 to "isinstance" has incompatible type "_SpecialForm"; expected "Union[type, Tuple[Union[type, Tuple[Any, ...]], ...]]"  [arg-type]

Which is referring to the following lines:

49 def _get_value_calculator(self, value_calculator: Union[str, Callable]):
50    if isinstance(value_calculator, str):
51        return self._calculators[value_calculator]
52    elif isinstance(value_calculator, Callable):
53        return value_calculator
54    else:
55        raise ValueError(
56            f"value_calculator must be either a string from {list(self._calculators.keys())} or a Callable."
57        )

Thanks!

[wyli]

I removed a few typing then it works fine...I think we can keep it simple for now

[danieltudosiu]

Thanks, I will need to understand later what the error really was.

[danieltudosiu]

@wyli I have a precision problem with torch.testing.assert_allclose.

If I manually check the get_value at this location:
https://github.com/Project-MONAI/MONAI/pull/1949/files#diff-3875231bae9af2fe3c36a3f2765da9ed1b129142fb62f15be036e2f1085e35cdR51

I get 0.333..... but somehow the assert_allclose does not pass because of the following error:

AssertionError: With rtol=0.0 and atol=0.001, found 1 element(s) (out of 1) whose difference(s) exceeded the margin of error (including 0 nan comparisons). The greatest difference was 0.0033333301544189453 (3.3333332538604736 vs. 3.3299999237060547), which occurred at index 0.

Which tells me that there is a precision issue.

Could you please tell me what I do wrong? Because not even the actual value that the assert_allclose get seems to be wrong even if I use the same exact method to get it.

[wyli]

@wyli I have a precision problem with torch.testing.assert_allclose.

If I manually check the get_value at this location:
https://github.com/Project-MONAI/MONAI/pull/1949/files#diff-3875231bae9af2fe3c36a3f2765da9ed1b129142fb62f15be036e2f1085e35cdR51

I get 0.333..... but somehow the assert_allclose does not pass because of the following error:

AssertionError: With rtol=0.0 and atol=0.001, found 1 element(s) (out of 1) whose difference(s) exceeded the margin of error (including 0 nan comparisons). The greatest difference was 0.0033333301544189453 (3.3333332538604736 vs. 3.3299999237060547), which occurred at index 0.

Which tells me that there is a precision issue.

Could you please tell me what I do wrong? Because not even the actual value that the assert_allclose get seems to be wrong even if I use the same exact method to get it.

changed the expected to 3.333333 looks like it works...

[wyli]

thanks

[vfdev-5]

@wyli @danieltudosiu nice PR and it makes me think of other schedules like piecewise or concat multiple schedules. Would you, guys, be interested in that if we promote ignite's contrib param scheduler to the core part such that MONAI could also use them ? We were also thinking about that and maybe some of more or less stable parts could be merged on our side.

[danieltudosiu]

Disclaimer: I am just a Ph.D. student being annoyed by tiny features that are missing from MONAI so I am by no means a developer with any voting right on this XD.

My two cents:

@vfdev-5 just to make sure we are on the same page, this ParameterScheduler is more of a general-purpose parameter scheduler for things like EMA decays in VQ-VAEs, temperatures for gumbel softmax, etc. Basically anything else BUT the learning rate.

For learning rate, MONAI has the LRScheduler handler which is less advanced than what you present in param scheduler.

From my point of view, having an Ignite handler should trump a MONAI handler since MONAI should aim to mainly have specialized code for Medical stuff. So you promoting the Learning Rate handlers to would fall in like with that.

Additionally moving the ParameterScheduler to ignite will also be a better place than keeping it in MONAI, and seems to fall in line with your ParamScheduler. Looking at your codebase it seems that you (as I did) use the closed forms of the schedulers, if you promote them then in the transition it would be a good idea to just generalize it to non-optimizer parameters so that all that logic can be reused in other settings.

[vfdev-5]

I think providing an example of usage would be of a great help for the users.

[danieltudosiu]

I completely forgot about the use-case example. I will do another pull request to amend this one.

[vfdev-5]

@danieltudosiu thanks for the clarification ! Actually, you are right, I missed that part about your parameter scheduler is not related to the optimizer as the module in ignite (btw, it is not only about LR, but momentum and other optimizer's params). I rechecked your code and also agree that this could be also a nice addition to ignite itself as well, as we also have use-cases like that to schedule some hyperparameters...

By promoting I was just mentioning the fact that currently MONAI does not use any ignite's contrib submodules, so experiment tracking and param scheduling, time profilers are out of the scope for MONAI. So, I was just saying that from ignite's side we were thinking to move some independent parts of contrib module to ignite's core module. This could also make it available for MONAI.

[danieltudosiu]

@vfdev-5 Sorry that I missed the extended usability of your LR schedulers, I just had a quick glance at them.

Regarding the contrib package, I think MONAI does not use them because they are in contrib so their future is uncertain. But @wyli can expand on this since he is one of the lead developers.

From my point of view I think having the ParamScheduler and its children moved of contrib and their logic merged with the ParameterScheduler to have a more general-purpose class would be a good move.

@vfdev-5 if you give the green light to the move I would be more than happy to lead the move since I want to start contributing to Ignite as well.

[vfdev-5]

Regarding the contrib package, I think MONAI does not use them because they are in contrib so their future is uncertain. But @wyli can expand on this since he is one of the lead developers.

Yes, I totally understand that and agree with that. We are planning to merge it inside the core part to avoid confusions etc.

@vfdev-5 if you give the green light to the move I would be more than happy to lead the move since I want to start contributing to Ignite as well.

@danieltudosiu yes, please go for it 👍 green light. I'll open an issue for that on ignite's side and can ping you there...

[danieltudosiu]

@danieltudosiu yes, please go for it 👍 green light. I'll open an issue for that on ignite's side and can ping you there...

Perfect, thanks!