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

updated test_param_scheduler replacing multiple calls with pytest.parametrize #2531

Merged
merged 3 commits into from
Mar 29, 2022

Conversation

Ishan-Kumar2
Copy link
Contributor

@Ishan-Kumar2 Ishan-Kumar2 commented Mar 27, 2022

Addresses #2522

Description:
Updated handlers/test_param_scheduler replacing multiple calls with pytest.parametrize

Haven't converted

def test_simulate_and_plot_values():

and
def test_scheduler_with_param_groups():

because it has conditional calls. We could have different fixtures for all the cases but I believe that would make the code less readable, let me know what you think would be best for these tests.
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)

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Mar 27, 2022

@Ishan-Kumar2 thanks for the PR, I left a comment to address.

We could have different fixtures for all the cases but I believe that would make the code less readable, let me know what you think would be best for these tests.

I agree, let's keep them as they are now...

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.

@Ishan-Kumar2 LGTM, thanks for the PR !

@vfdev-5 vfdev-5 enabled auto-merge (squash) March 29, 2022 11:23
@vfdev-5 vfdev-5 merged commit da03a39 into pytorch:master Mar 29, 2022
@Ishan-Kumar2 Ishan-Kumar2 deleted the pytest_parameterize branch March 29, 2022 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants