Misleading documentation for OneCycleLR, LambdaLR

[cowwoc]

Version 1.4.4

Per #4929 (comment) OneCycleLR, LambdaLR require configuration "interval": "step" which should be set as follows:

  'lr_scheduler': {
          'scheduler': scheduler, 
          'interval': 'step'
  }

But https://pytorch-lightning.readthedocs.io/en/latest/api/pytorch_lightning.core.lightning.html#pytorch_lightning.core.lightning.LightningModule.configure_optimizers implies it should be set as follows:

  'lr_scheduler': scheduler, 
  'interval': 'step'

I can confirm the first approach works. The documentation in the second link should be reviewed. I suspect it is incorrect.

[tchaton]

Hey @cowwoc,

Mind sending a screenshot where you see the second one ?

I can see only recommendations for the first one.

[SkafteNicki]

The documentation seems correct, but maybe a bit convoluted. If you return a dictionary from configure_optimizer then the key which holds the scheduler should be lr_scheduler. But if you then want to pass additional parameters for the learning rate scheduler, you need a new dict (so a dict inside a dict) where scheduler is the actual scheduler class (please see the image below from the docs)

[cowwoc]

@tchaton @SkafteNicki You are right. I was thrown off by this part of the documentation:

The lr_dict is a dictionary which contains the scheduler and its associated configuration. The default configuration is shown below.

lr_dict = {
    # REQUIRED: The scheduler instance
    "scheduler": lr_scheduler,
    # The unit of the scheduler's step size, could also be 'step'.
    # 'epoch' updates the scheduler on epoch end whereas 'step'
    # updates it after a optimizer update.
    "interval": "epoch",
    # How many epochs/steps should pass between calls to
    # `scheduler.step()`. 1 corresponds to updating the learning
    # rate after every epoch/step.
    "frequency": 1,
    # Metric to to monitor for schedulers like `ReduceLROnPlateau`
    "monitor": "val_loss",
    # If set to `True`, will enforce that the value specified 'monitor'
    # is available when the scheduler is updated, thus stopping
    # training if not found. If set to `False`, it will only produce a warning
    "strict": True,
    # If using the `LearningRateMonitor` callback to monitor the
    # learning rate progress, this keyword can be used to specify
    # a custom logged name
    "name": None,
}

The fact that "interval": "epoch" is shown on the same level as "scheduler": lr_scheduler led me to incorrectly assume that I was supposed to return the second form.

I rarely read the entire page from top to bottom so when scanning quickly for how to configure OneCycleLR I saw this section first and assumed this was the correct form. The example code listed below is much clearer in my opinion. Maybe it would help to show the example code before listing the default values? And perhaps rename lr_dict to lr_scheduler_config to make clearer that it's a nested value of lr_scheduler.

[tchaton]

Mind making a PR with the following change:

{"lr_scheduler": {"scheduler": scheduler, ....}}

I believe it would be more consistent to use lr_scheduler as the main key, and only scheduler for both key and variable name inside.

[cowwoc]

@tchaton I ended up naming it lr_scheduler_config to avoid variable name conflicts in the tests. So lr_scheduler_config refers to the config while lr_scheduler refers to the scheduler object.

See #9313