[feat] Named Parameter Groups in `LearningRateMonitor` by AffineParameter · Pull Request #7987 · Lightning-AI/pytorch-lightning

AffineParameter · 2021-06-15T12:21:16Z

What does this PR do?

This PR allows the LearningRateMonitor callback to log the actual parameter group names, if they exist. It should fallback to prior behavior in the event that the parameter group names do not exist.

Fixes #7981

Before submitting

Was this discussed/approved via a GitHub issue? (not for typos and docs)
Did you read the contributor guideline, Pull Request section?
Did you make sure your PR does only one thing, instead of bundling different changes together?
Did you make sure to update the documentation with your changes? (if necessary)
Did you write any new necessary tests? (not for typos and docs)
Did you verify new and existing tests pass locally with your changes?
Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self-explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

pep8speaks · 2021-06-15T12:21:19Z

Hello @AffineParameter! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-06-16 18:39:55 UTC

for more information, see https://pre-commit.ci

codecov · 2021-06-15T12:24:01Z

Codecov Report

Merging #7987 (9d1af7e) into master (f7459f5) will decrease coverage by 5%.
The diff coverage is 91%.

@@           Coverage Diff           @@
##           master   #7987    +/-   ##
=======================================
- Coverage      92%     87%    -5%     
=======================================
  Files         203     207     +4     
  Lines       13157   13389   +232     
=======================================
- Hits        12043   11617   -426     
- Misses       1114    1772   +658

for more information, see https://pre-commit.ci

tchaton

LGTM !

for more information, see https://pre-commit.ci

AffineParameter · 2021-06-16T15:19:17Z

Happy to update the CHANGELOG, provided I can get a milestone to target. This should tie a bow on things?

Thanks for the quick reviews!

awaelchli · 2021-06-16T15:44:53Z

@AffineParameter (love the name btw) <3
This is a feature so it will go into 1.4. Could you add the changelog entry to the 1.4 section then in CHANGELOG.md? Then it is ready for merge :)

AffineParameter · 2021-06-16T16:03:37Z

PR Digression:

The contribution process (docs, workflow, reviews, etc.) have been really easy to work with! I am sure getting to this point hasn't been easy, but the effort and intentionality really shows. 🎩 's to all those responsible! :)

AffineParameter · 2021-06-16T16:49:44Z

This is the last time I draw more attention to my already approved PR's by complementing the maintainers... 🤣

But seriously, I'll respond when I get the chance, and thanks for the great comments & suggestions!

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>

tchaton · 2021-06-16T18:22:13Z

PR Digression:

The contribution process (docs, workflow, reviews, etc.) have been really easy to work with! I am sure getting to this point hasn't been easy, but the effort and intentionality really shows. 🎩 's to all those responsible! :)

Hey @AffineParameter, we definitely appreciated your mindset and contribution !
We hope to see more contributions from you and hope to keep the contribution process at the same high standard.

Best,
T.C

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>

AffineParameter added 3 commits June 14, 2021 18:09

let LearningRateMonitor use parameter_group names

7c4ddf4

update tests

cb0b9e3

update docs

5177dcf

[pre-commit.ci] auto fixes from pre-commit.com hooks

cce0041

for more information, see https://pre-commit.ci

AffineParameter and others added 5 commits June 15, 2021 07:29

clean up after linter

d9e717e

[pre-commit.ci] auto fixes from pre-commit.com hooks

71fdf78

for more information, see https://pre-commit.ci

rename the new unit test 🤦

3d34575

attempt to head-fake the auto-linter

df7fd3a

[pre-commit.ci] auto fixes from pre-commit.com hooks

2decdb6

for more information, see https://pre-commit.ci

AffineParameter commented Jun 15, 2021

View reviewed changes

Comment thread tests/callbacks/test_lr_monitor.py Outdated

next attempt to satisfy the linter

44ac6af

AffineParameter marked this pull request as ready for review June 15, 2021 16:01

AffineParameter requested review from Borda, SeanNaren, awaelchli, carmocca, justusschock, kaushikb11, tchaton and williamFalcon as code owners June 15, 2021 16:01

AffineParameter changed the title ~~Named Parameter Groups in LearningRateMonitor~~ [feat] Named Parameter Groups in LearningRateMonitor Jun 15, 2021

awaelchli added the feature Is an improvement or enhancement label Jun 15, 2021

tchaton approved these changes Jun 16, 2021

View reviewed changes

justusschock approved these changes Jun 16, 2021

View reviewed changes

awaelchli approved these changes Jun 16, 2021

View reviewed changes

Comment thread pytorch_lightning/callbacks/lr_monitor.py Outdated

tchaton added the ready to be merged PRs ready to be merged label Jun 16, 2021

AffineParameter and others added 2 commits June 16, 2021 09:19

Respond to review comments & adjust if/else

5ca983b

[pre-commit.ci] auto fixes from pre-commit.com hooks

666e303

for more information, see https://pre-commit.ci

awaelchli added this to the v1.4 milestone Jun 16, 2021

Update CHANGELOG.md

41c301f

awaelchli enabled auto-merge (squash) June 16, 2021 16:06

kaushikb11 reviewed Jun 16, 2021

View reviewed changes

Comment thread pytorch_lightning/callbacks/lr_monitor.py

kaushikb11 disabled auto-merge June 16, 2021 16:25

Update pytorch_lightning/callbacks/lr_monitor.py

d46fd5e

kaushikb11 enabled auto-merge (squash) June 16, 2021 16:27

carmocca disabled auto-merge June 16, 2021 16:37

carmocca reviewed Jun 16, 2021

View reviewed changes

AffineParameter and others added 5 commits June 16, 2021 11:55

Update pytorch_lightning/callbacks/lr_monitor.py

0d14952

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>

Update pytorch_lightning/callbacks/lr_monitor.py

ff59320

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>

Update tests/callbacks/test_lr_monitor.py

c13ae59

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>

remove unnecessary training/val loops during unit tests

40181c8

revert suggested simplification & use 1-indexing in enumerate

c729971

tchaton enabled auto-merge (squash) June 16, 2021 18:19

Update pytorch_lightning/callbacks/lr_monitor.py

9d1af7e

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>

auto-merge was automatically disabled June 16, 2021 18:39
Head branch was pushed to by a user without write access

carmocca merged commit 906de2a into Lightning-AI:master Jun 17, 2021

Conversation

AffineParameter commented Jun 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Before submitting

PR review

Did you have fun?

Uh oh!

pep8speaks commented Jun 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2021-06-16 18:39:55 UTC

Uh oh!

codecov Bot commented Jun 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

tchaton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AffineParameter commented Jun 16, 2021

Uh oh!

awaelchli commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AffineParameter commented Jun 16, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AffineParameter commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tchaton commented Jun 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

AffineParameter commented Jun 15, 2021 •

edited

Loading

pep8speaks commented Jun 15, 2021 •

edited

Loading

codecov Bot commented Jun 15, 2021 •

edited

Loading

awaelchli commented Jun 16, 2021 •

edited

Loading

AffineParameter commented Jun 16, 2021 •

edited

Loading