FIX Makes sure score is passed to ReduceLROnPlateau #738
Conversation
CHANGES.md
Outdated
| @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| - CLI helper function now also supports normal (i.e. non-skorch) sklearn estimators | |||
| - Disabling all callbacks is now supported (which allows reducing overhead, | |||
| which is especially relevant for small models). | |||
| - `LRScheduler` now currently passes the score/loss in the history to `ReduceLROnPlateau`. (#738) | |||
There was a problem hiding this comment.
Not sure if I understand this sentence correctly, maybe change the wording?
skorch/callbacks/lr_scheduler.py
Outdated
| score = np.inf | ||
| else: | ||
| score = net.history[-1, self.monitor] | ||
| score = net.history[-1, self.monitor] |
There was a problem hiding this comment.
What do you think: We could catch the KeyError and augment the error message by telling the user that they may possibly need to change the order of callbacks if they reference a metric that was not yet computed?
There was a problem hiding this comment.
I think its a good idea. I updated the PR to include this nicer error message.
| f"'{self.monitor}' was not found in history. A " | ||
| f"PassthroughScoring callback with name='{self.monitor}' " | ||
| "should be placed before the LRScheduler callback" | ||
| ) from e | ||
|
|
||
| self.lr_scheduler_.step(score) | ||
| # ReduceLROnPlateau does not expose the current lr so it can't be recorded |
There was a problem hiding this comment.
Thanks for the fix in this PR, which I found while troubleshooting the same unexpected behavior as mentioned in the issue #363.
I noticed this comment but I guess how the learning rate was updated should be tracked in net.history even for ReduceLROnPlateau. As being discussed in https://discuss.pytorch.org/t/how-to-retrieve-learning-rate-from-reducelronplateau-scheduler/54234/3 and pytorch/pytorch#2829, I would suggest replacing the comment line 162 with the following:
if self.event_name is not None:
net.history.record(self.event_name, self.lr_scheduler_._last_lr[0]) # type: ignore
or
if self.event_name is not None:
net.history.record(self.event_name, self.lr_scheduler_.optimizer.param_groups[0]["lr"]) # type: ignore
There was a problem hiding this comment.
We can discuss this in another PR since this is a feature unrelated to the bug fix.
There was a problem hiding this comment.
I created an issue (#739) so that we don't forget. @jiajiexiao does that reflect what you meant?
There was a problem hiding this comment.
Sure. Thanks, @thomasjpfan @BenjaminBossan
Co-authored-by: Benjamin Bossan <[email protected]>
FIX Makes sure score is passed to ReduceLROnPlateau (skorch-dev#738)
Closes #363