Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix lr key name in case of param groups #1719

Merged
merged 4 commits into from
May 10, 2020
Merged

Fix lr key name in case of param groups #1719

merged 4 commits into from
May 10, 2020

Conversation

rohitgr7
Copy link
Contributor

@rohitgr7 rohitgr7 commented May 3, 2020

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Fix lr key name in case of param groups.

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@mergify mergify bot requested a review from a team May 3, 2020 19:59
@awaelchli
Copy link
Member

ping @SkafteNicki for revew :)

@codecov
Copy link

codecov bot commented May 3, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@0cb6767). Click here to learn what that means.
The diff coverage is 100%.

@@           Coverage Diff            @@
##             master   #1719   +/-   ##
========================================
  Coverage          ?     88%           
========================================
  Files             ?      69           
  Lines             ?    4302           
  Branches          ?       0           
========================================
  Hits              ?    3793           
  Misses            ?     509           
  Partials          ?       0           

@Borda Borda added the bug Something isn't working label May 3, 2020
@Borda Borda added this to the 0.7.6 milestone May 3, 2020
@Borda Borda added the ready PRs ready to be merged label May 3, 2020
@mergify mergify bot requested a review from a team May 3, 2020 20:24
@SkafteNicki
Copy link
Member

LGTM

@Borda
Copy link
Member

Borda commented May 4, 2020

it seems that these lines are not used in any test, may you pls add test for it?
btw, @williamFalcon it LGTM 🐰

@pep8speaks
Copy link

pep8speaks commented May 4, 2020

Hello @rohitgr7! Thanks for updating this PR.

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

Comment last updated at 2020-05-10 18:13:11 UTC

tests/callbacks/test_callbacks.py Outdated Show resolved Hide resolved
@mergify mergify bot requested a review from a team May 4, 2020 20:28
@mergify
Copy link
Contributor

mergify bot commented May 4, 2020

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

@Borda Borda removed the ready PRs ready to be merged label May 4, 2020
Copy link
Member

@awaelchli awaelchli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!
Just a bugfix note in the changelog is missing :)

@mergify mergify bot requested a review from a team May 6, 2020 01:31
@mergify
Copy link
Contributor

mergify bot commented May 6, 2020

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

@rohitgr7 rohitgr7 requested review from Borda and removed request for a team May 9, 2020 13:50
@mergify mergify bot requested a review from a team May 9, 2020 13:50
@Borda Borda added the ready PRs ready to be merged label May 9, 2020
@mergify mergify bot requested a review from a team May 9, 2020 14:30
@mergify
Copy link
Contributor

mergify bot commented May 10, 2020

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

@williamFalcon
Copy link
Contributor

@Borda rebase?

@Borda
Copy link
Member

Borda commented May 10, 2020

@Borda rebase?

@williamFalcon done

@williamFalcon williamFalcon merged commit d962ab5 into Lightning-AI:master May 10, 2020
@rohitgr7 rohitgr7 deleted the patch-1 branch May 10, 2020 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants