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.

Sign up for GitHub

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

Jump to bottom

Change LinearCyclicalScheduler to triangle wave to sawtooth wave #3186

Merged
merged 14 commits into from
Feb 8, 2024
Merged

Change LinearCyclicalScheduler to triangle wave to sawtooth wave #3186

merged 14 commits into from
Feb 8, 2024

Conversation

sihyeong671
Copy link
Contributor

@sihyeong671 sihyeong671 commented Jan 11, 2024 •
edited by vfdev-5
Loading

Fixes #3185

Description:

Add monotonic variable in LinearCyclicalScheduler
LinearCyclicalScheduler's get_param method is modified

Add Test case LinearCyclicalScheduler ValueError
Add Test case LinearCyclicalScheduler with warmup duration

Check list:

  • New tests are added (if a new feature is added)
  • New doc strings: description and/or example code are in RST format
  • Documentation is updated (if required)

@github-actions github-actions bot added the module: handlers Core Handlers module label Jan 11, 2024
@vfdev-5
Copy link
Collaborator

vfdev-5 commented Jan 11, 2024

Thanks for the PR and opening a corresponding feature request, @sihyeong671 !
The only problem I see here is BC-breaking change, so let's implement this feature as an optional flag with default value keeping current behavior, such that ignite users won't have their code broken after this PR get landed. What do you think?

@sihyeong671
Copy link
Contributor Author

sihyeong671 commented Jan 11, 2024 •
edited
Loading

Thanks for the PR and opening a corresponding feature request, @sihyeong671 ! The only problem I see here is BC-breaking change, so let's implement this feature as an optional flag with default value keeping current behavior, such that ignite users won't have their code broken after this PR get landed. What do you think?

got it! I think that is a good idea.I will try add optional flag

@sihyeong671
feat: change linearcyclicalscheduler
8d68161 
add optional flag for triangle and sawtooth wave
@sihyeong671
feat: fix LinearCyclicalScheduler
d091eea 
- move value error location to __init__
- rename use_legacy to use_sawtooth
@sihyeong671
test: add LinearCyclicalScheduler using use_sawtooth option test
6d10fde
@sihyeong671
Merge branch 'feat/scheduler' of https://github.com/sihyeong671/ignite …
75c3399 
…into feat/scheduler
@sihyeong671
Revert "test: add LinearCyclicalScheduler using use_sawtooth option t…
c3b88e0 
…est"

This reverts commit 6d10fde.
@sihyeong671
Merge branch 'master' of https://github.com/pytorch/ignite into feat/…
831db8d 
…scheduler
@sihyeong671
feat: Modify LinearCyclicalScheduler
86b7fed 
- add monotonic variable
- check Error case
@sihyeong671
test: add test case
22da23b 
- warmup_duration, monotonic Value Error Check
- add test case for linear cyclical scheduler with warmup_duration
@sihyeong671
fix: remove #noqa E501, split sentence
22d0475
vfdev-5
vfdev-5 approved these changes Feb 8, 2024
View reviewed changes
Copy link
Collaborator

@vfdev-5 vfdev-5 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 to me, thanks a lot for the contribution, @sihyeong671 !

@vfdev-5 vfdev-5 merged commit 7fefe60 into pytorch:master Feb 8, 2024
14 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vfdev-5 vfdev-5 vfdev-5 approved these changes

Assignees
No one assigned
Labels
module: handlers Core Handlers module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Changing LinearCyclicalScheduler triangle wave to sawtooth wave
2 participants
@sihyeong671 @vfdev-5