Repository navigation
Fix ReduceLROnPlateauScheduler ignoring min_lr - #3876
G26karthik wants to merge 4 commits into
Conversation
min_lr was replaced by 0 when param_group_index is None, and with param_group_index > 0 the floor was stored at an index _reduce_lr never reads. Pass min_lr to torch's ReduceLROnPlateau unchanged; torch expands a float to every param group.
|
Could we extend this test with a few additional plateau metrics after the LR reaches |
|
Thanks for the review! Done in d3668ab. The test now runs four more plateau evaluations after the LR reaches 0.3 and checks that it stays there, and it compares the full LR history of every param group the scheduler updates, so with |
|
Hi @vfdev-5, sorry to bother you! This one was approved by @aaishwarymishra on Oct 7 and CI is green. Could you take a look when you get a chance? #3880 is a related fix for the same scheduler (its |
| min_lr = scheduler_kwargs.get("min_lr", 0.0) | ||
| if param_group_index is not None and not isinstance(min_lr, float): | ||
| raise TypeError(f"When param_group_index is given, min_lr should be a float, but given {type(min_lr)}") | ||
| _scheduler_kwargs = scheduler_kwargs.copy() |
There was a problem hiding this comment.
Do we still need to make a copy now?
There was a problem hiding this comment.
No, good point. It was only there so overwriting min_lr wouldn't touch the caller's dict, and **scheduler_kwargs is already a fresh dict, so setting verbose on it is fine too. Removed it in 9832774.
Fixes #3875
Description:
ReduceLROnPlateauSchedulerreplacedmin_lrwith 0 whenparam_group_indexis None, and withparam_group_index=k > 0it put the floor at index k while_reduce_lrreadsmin_lrs[0]for the single selected group. In both cases the LR decayed pastmin_lr.This PR stops rewriting
min_lrand passes it to torch'sReduceLROnPlateauas given. torch expands a float to one value per param group, so_reduce_lrreads the right floor whether one group or all groups are scheduled. TheTypeErrorfor a non-floatmin_lrtogether withparam_group_indexstays. A listmin_lrwithoutparam_group_indexnow reaches torch too, instead of being replaced by 0.For the test, I parametrized the existing
test_reduce_lr_on_plateau_scheduleroverparam_group_index(None, 0, 1) and raised itsmin_lrfrom 1e-7 to 0.3, so the last reduction now hits the floor (0.25 is clamped to 0.3). The old 1e-7 was never reached, which is why the test didn't catch this. On master the None and 1 cases fail (they end at 0.25), and with the fix all 6 cases pass. The rest oftests/ignite/handlers/test_param_scheduler.pypasses locally (65 passed;test_simulate_and_plot_valuesneeds matplotlib, which isn't in my env), and ruff check/format are clean.Check list: