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

Make KneeLRScheduler compatible PyTorch scheduler format #1

Merged
merged 2 commits into from Jun 9, 2021

Conversation

Animatory
Copy link

KneeLRScheduler now inherits _LRScheduler, supports momentum for Adam and SGD. Add non-zero start lr and non-zero final lr. Add different ways to set up a number of training steps as it is done in OneCycleLR.
Add documentation for the class.

@nikhil-iyer-97
Copy link
Owner

Mighty thanks! Let me spend a few hours going through this PR. Will approve it as soon as I test it on a few testcases and everything works out.

@nikhil-iyer-97 nikhil-iyer-97 self-requested a review June 9, 2021 12:49
@nikhil-iyer-97 nikhil-iyer-97 added the enhancement New feature or request label Jun 9, 2021
@nikhil-iyer-97 nikhil-iyer-97 changed the base branch from main to test June 9, 2021 15:59
infer the total number of steps in the cycle if a value for total_steps is not provided.
- `pct_start` (float): The percentage of the total steps spent increasing the learning rate.
- `pct_explore` (float): The percentage of the total steps spent on explore phase (keeping max the learning rate).
- `cycle_momentum` (bool): If ``True``, momentum is cycled inversely
Copy link
Owner

Choose a reason for hiding this comment

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

not sure if we want to provide cyclical momentum . No idea how it will affect the explore phase and the effect it may have on reaching a wider minima

Copy link
Author

Choose a reason for hiding this comment

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

So, it will be better to set this parameter False by default



class KneeLRScheduler(_LRScheduler):
Copy link
Owner

Choose a reason for hiding this comment

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

importing _LRScheduler is a good idea, missed it out initially . Will have to change the args as we dont want users to pass in many parameters. Main parameters ( explore , total steps or total epochs , warmup steps). Remaining can be the same as other schedulers in pytorch.

Copy link
Author

Choose a reason for hiding this comment

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

Actually, there are only 3-4 required args. The rest of args provide customizability of schedule

class KneeLRScheduler(_LRScheduler):
"""
`Wide-minima Density Hypothesis and the Explore-Exploit Learning Rate Schedule`
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for citing the work

"""

def __init__(self, optimizer: Optimizer, max_lr: Union[List[float], float], total_steps: int = None,
Copy link
Owner

Choose a reason for hiding this comment

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

thanks for importing and using typing. Helps with documentation and readability

Copy link
Owner

@nikhil-iyer-97 nikhil-iyer-97 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, merging into test branch for now. Will take this up and make necessary edits

@nikhil-iyer-97 nikhil-iyer-97 merged commit 35366d8 into nikhil-iyer-97:test Jun 9, 2021
@nikhil-iyer-97 nikhil-iyer-97 added the good first issue Good for newcomers label Jun 9, 2021
@Animatory
Copy link
Author

Heard of you from the issue on PyTorch. Good job!

@nikhil-iyer-97
Copy link
Owner

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants