Lightning-AI/pytorch-lightning

Refactor LearningRateMonitor

Open

#17,613 opened on May 11, 2023

View on GitHub
 (2 comments) (0 reactions) (0 assignees)Python (3,233 forks)batch import
callback: lr monitorfeaturehelp wantedrefactor

Repository metrics

Stars
 (26,687 stars)
PR merge metrics
 (Avg merge 9d 15h) (3 merged PRs in 30d)

Description

Outline & Motivation

  • self.last_momentum_values does not seem needed, but I may be overlooking something.
  • Instead of returning the name with -momentum suffix when logging betas, just log it with -beta suffix.
  • If logging betas, log all of them. The name could therefore be -beta0, -beta1 in the case of Adam.
  • Change the current logging names, e.g., lr-Adam to optimizer-Adam-lr and lr-Adam-momentum to optimizer-Adam-momentum (it doesn't have momentum this is just for demonstration, it would actually be optimizer-Adam-beta0, optimizer-Adam-beta1).
  • remove the log_momentum argument as it can be inferred automatically.

Pitch

Happy to open a PR with these changes.

Additional context

No response

cc @borda @justusschock @awaelchli

Contributor guide

Research direction
Examine the current LearningRateMonitor implementation, understand the logging logic for betas and momentum, refactor according to the proposed changes, and ensure backward compatibility is handled.
Tech stack
python
Domain
machine learningaibackend
Issue type
Refactor
DifficultyEstimated implementation difficulty for a new contributor, from 1 for very small changes to 5 for expert-level work.
2
Estimated timeA rough time range for an experienced contributor to investigate, implement, test, and prepare a pull request.
Half day
Activity statusHow available the issue appears right now: fresh, active, stale, blocked, or waiting on maintainer input.
Active
ClarityHow clearly the issue explains the expected change, acceptance criteria, and next step.
Clear
Prerequisites
PythonPyTorch LightningGit
Newbie friendlinessA 1-100 score estimating how approachable this issue is for first-time contributors.
70