This pull request is now in conflict... :(

Codecov Report

Merging #1498 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1498   +/-   ##
======================================
  Coverage      89%     89%           
======================================
  Files          72      72           
  Lines        4229    4229           
======================================
  Hits         3744    3744           
  Misses        485     485           

This pull request is now in conflict... :(

Thank you for this extremely useful callback, @SkafteNicki !

I've noticed that you are using d = dict.fromkeys in lr_logger.py#L62 and later use d[key].append(...) on lr_logger.py#L84-L87. I think L62 is incorrect, since L84-L87 would appends to all lists at once, because the dictionary created at line L62 contains reference to the same list. The following example demonstrates the issue:

names = ['a', 'b', 'c']

d1 = dict.fromkeys(names, [])
d2 = {k: [] for k in names}

d1['a'].append(1)
d1['c'].append(2)

d2['a'].append(1)
d2['c'].append(2)

print(f'd1 = {d1}')
print(f'd2 = {d2}')

prints

d1 = {'a': [1, 2], 'b': [1, 2], 'c': [1, 2]}
d2 = {'a': [1], 'b': [], 'c': [2]}

Contents of d1 do not seem to be the behaviour desired by the logic of ._extract_lr().

@ivannz good catch, I was not aware that fromkeys should not be used in combination with mutable objects. Could you do a PR to correct this?

@SkafteNicki Thank you, done.

As a sidenote: in my experiments I found it quite useful to disable the misconfiguration check, since it requires an inconvenient branching (to remove LearningRateLogger) before a call to trainer when no scheduler is present.

What was the rationale behind issuing an MisconfigurationException rather than a warning, that would still allow for the trainer to continue?

@ivannz not really any rational behind, I just assumed that everyone that used this has a scheduler in their model. However, I can clearly see that it is smarter to have a warning instead of an exception. Could you also add this to the PR?

@SkafteNicki done. I am going to patch both in a single PR.

how to access the LR logged by LearningRateLogger? i couldn't find an example from the docs.

@yitang the learning rate(s) are stored in the the lrs attribute of the LearningRateLogger

@SkafteNicki is this written in the docs?

@williamFalcon No, as the (originally) intention of the LearningRateLogger is that the lrs are logged as any other scalar to the trainer.logger, thus the user could interact with the values in that way. However, maybe it could be made clear in the docs that they are also stored in the lrs attribute.