-
-
Notifications
You must be signed in to change notification settings - Fork 619
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
feat: Added warmup each cycle feature in CyclicalScheduler #3064
Conversation
- add _get_cycle_param method in CyclicalScheduler - add warmup_each_cycle, warmup_duration variable in CyclicalScheduler - add warmup phase in CosineAnnealingScheduler issue pytorch#3036
- remove f in not using f string variable sentence
- rename get_param in LinearCyclicalScheduler, CosineAnnealingScheduler to _get_clcye_param
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @sihyeong671 , I left some comments.
- fix typo - add docsting which is in PR review - change _get_cycle_param to abstractmethod - raise ValueError when warmup_each_cycle=True && warmup_duration is None
- add test_cyclical_scheduler_asserts - add test_cosine_annealing_scheduler_warmup
@sihyeong671 thanks for working on this PR! Can you please fix this test issue:
|
In macOS, If I change the sentence, AssertionError like below code
|
@sihyeong671 I see, let's just check for this sting "Can't instantiate abstract class CyclicalScheduler": with pytest.raises(
TypeError, match="Can't instantiate abstract class CyclicalScheduler"
):
CyclicalScheduler({}, "lr", 0.0, 1.0, 10) |
@vfdev-5 I have no idea how to fix the error. In cpu-tests, error shows "Error: The operation was canceled.", how to handle it? |
@sihyeong671 no worries, this can be a flaky CI. We set 45 minutes limit if the test stuck with distributed tests. So, just restarting it. |
- change _get_cycle_param to _get_param - add _get_param in ParamScheduler - remove warmup_each_cycle variable
- remove test_cyclical_scheduler - remove warmup_each_cycle variable
@sihyeong671 can you please fix mypy issue: https://github.com/pytorch/ignite/actions/runs/6231400451/job/16912898300?pr=3064#step:12:18 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @sihyeong671 and @sadra-barikbin !
issue #3036
Description:
this feature is following https://github.com/katsura-jp/pytorch-cosine-annealing-with-warmup
Check list: