Skip to content

Commit

Permalink
Suggested LR not returned when min_grad_idx is 0 in plot() (#66)
Browse files Browse the repository at this point in the history
`min_grad_idx` is a numerical value but it was not properly used to
determined whether suggested lr should be returned through the
following statement:
```python
if suggest_lr and min_grad_idx:
```

As @manuel-munoz-aguirre mentioned in issue #65, this IF-statement
will fail to return suggested lr when `min_grad_idx` is 0.

A proper solution for this issue should be explicitly checking
whether `min_grad_idx` is `None` or not (as it was initialized to
`None` in line 503).
  • Loading branch information
NaleRaphael authored Nov 5, 2020
1 parent 9cfcbec commit acc5e7e
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 1 deletion.
18 changes: 18 additions & 0 deletions tests/test_lr_finder.py
Original file line number Diff line number Diff line change
Expand Up @@ -372,3 +372,21 @@ def test_suggest_lr():
ax, lr = lr_finder.plot(skip_start=0, skip_end=0, suggest_lr=True, ax=ax)

assert lr == 2

# Loss with minimal gradient is the first element in history
lr_finder.history["loss"] = [1, 0, 1, 2, 3, 4]
lr_finder.history["lr"] = range(len(lr_finder.history["loss"]))

fig, ax = plt.subplots()
ax, lr = lr_finder.plot(skip_start=0, skip_end=0, suggest_lr=True, ax=ax)

assert lr == 0

# Loss with minimal gradient is the last element in history
lr_finder.history["loss"] = [0, 1, 2, 3, 4, 3]
lr_finder.history["lr"] = range(len(lr_finder.history["loss"]))

fig, ax = plt.subplots()
ax, lr = lr_finder.plot(skip_start=0, skip_end=0, suggest_lr=True, ax=ax)

assert lr == len(lr_finder.history["loss"]) - 1
2 changes: 1 addition & 1 deletion torch_lr_finder/lr_finder.py
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,7 @@ def plot(
if fig is not None:
plt.show()

if suggest_lr and min_grad_idx:
if suggest_lr and min_grad_idx is not None:
return ax, lrs[min_grad_idx]
else:
return ax
Expand Down

0 comments on commit acc5e7e

Please sign in to comment.