Skip to content

Conversation

sihyeong671
Copy link
Contributor

@sihyeong671 sihyeong671 commented Sep 16, 2023

  • add _get_param method in CyclicalScheduler
  • add warmup_duration variable in CyclicalScheduler

issue #3036

Description:
this feature is following https://github.com/katsura-jp/pytorch-cosine-annealing-with-warmup

import numpy as np
import matplotlib.pylab as plt
from ignite.handlers import CosineAnnealingScheduler


lr_values_1 = np.array(CosineAnnealingScheduler.simulate_values(
    num_events=300,
    param_name='lr',
    start_value=1.0, 
    end_value=0.0,
    cycle_size=100,
    warmup_duration=25,)
)


plt.figure(figsize=(12, 9))
plt.title("LinearCyclicalScheduler")
plt.plot(lr_values_1[:, 0], lr_values_1[:, 1], label="learning rate")
plt.xlabel("events")
plt.ylabel("values")
plt.legend()
plt.ylim([0.0, 1.1])
plt.show()

image

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)

- add _get_cycle_param method in CyclicalScheduler
- add warmup_each_cycle, warmup_duration variable in CyclicalScheduler
- add warmup phase in CosineAnnealingScheduler

issue pytorch#3036
@github-actions github-actions bot added the module: handlers Core Handlers module label Sep 16, 2023
- remove f in not using f string variable sentence
- rename get_param in LinearCyclicalScheduler, CosineAnnealingScheduler to _get_clcye_param
@sihyeong671 sihyeong671 requested a review from vfdev-5 September 17, 2023 08:27
Copy link
Collaborator

@sadra-barikbin sadra-barikbin left a 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
@vfdev-5
Copy link
Collaborator

vfdev-5 commented Sep 18, 2023

@sihyeong671 thanks for working on this PR! Can you please fix this test issue:

FAILED tests/ignite/handlers/test_param_scheduler.py::test_cyclical_scheduler_asserts - AssertionError: Regex pattern did not match.
 Regex: "Can't instantiate abstract class CyclicalScheduler with abstract method _get_cycle_param"
 Input: "Can't instantiate abstract class CyclicalScheduler with abstract methods _get_cycle_param"

@sihyeong671
Copy link
Contributor Author

@sihyeong671 thanks for working on this PR! Can you please fix this test issue:

FAILED tests/ignite/handlers/test_param_scheduler.py::test_cyclical_scheduler_asserts - AssertionError: Regex pattern did not match.
 Regex: "Can't instantiate abstract class CyclicalScheduler with abstract method _get_cycle_param"
 Input: "Can't instantiate abstract class CyclicalScheduler with abstract methods _get_cycle_param"

In macOS, If I change the sentence, AssertionError like below code
First, I will change it and PR

E           AssertionError: Regex pattern did not match.
E            Regex: "Can't instantiate abstract class CyclicalScheduler with abstract methods _get_cycle_param"
E            Input: "Can't instantiate abstract class CyclicalScheduler with abstract method _get_cycle_param"

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Sep 18, 2023

@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)

@sihyeong671 sihyeong671 requested a review from vfdev-5 September 18, 2023 09:19
@sihyeong671
Copy link
Contributor Author

sihyeong671 commented Sep 18, 2023

@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?

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Sep 18, 2023

@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
@vfdev-5
Copy link
Collaborator

vfdev-5 commented Sep 19, 2023

@sihyeong671 can you please fix mypy issue: https://github.com/pytorch/ignite/actions/runs/6231400451/job/16912898300?pr=3064#step:12:18

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.

LGTM, thanks @sihyeong671 and @sadra-barikbin !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: handlers Core Handlers module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants