Skip to content

Port Group O test to pytest ref #3945 #3955

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

Merged
merged 2 commits into from
Jun 3, 2021
Merged

Port Group O test to pytest ref #3945 #3955

merged 2 commits into from
Jun 3, 2021

Conversation

ShrillShrestha
Copy link
Contributor

Refactor Group O mentioned in #3945

  • test_random_invert
  • test_random_posterize
  • test_random_solarize
  • test_random_adjust_sharpness
  • test_random_autocontrast
  • test_random_equalize

Please let me know if I need to change anything.

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks @ShrillShrestha ! This looks great, I made some minor comment, LMK what you think

@@ -1834,6 +1764,83 @@ def test_pad_with_mode_F_images(self):
assert_equal(padded_img.size, [edge_size + 2 * pad for edge_size in img.size], check_stride=False)


def _test_randomness(fn, trans, configs):
Copy link
Member

Choose a reason for hiding this comment

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

This is fine as-is for me. If we want, we can go one step further about parametrization here. I'm thinking of somthing like this:

@pytest.mark.skipif(stats is None, reason="scipy.stats not available")
@pytest.mark.parametrize('fn, trans, conf', [
    (F.invert, transforms.RandomInvert, {}),
    (F.posterize, transforms.RandomPosterize, {"bits": 4}),
   ...

])
@pytest.mark.parametrize('p', (.5, .7))
def test_randomness(fn, trans, conf, p):

and we could get rid of all the other functions and the 2 for loops inside. WDYT?

Again this is optional: feel free to leave as-is if you prefer ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think, we can parametrize it. I have made the changes.

@NicolasHug
Copy link
Member

+30 −70

Nice!

Thanks a lot @ShrillShrestha !

@ShrillShrestha
Copy link
Contributor Author

@NicolasHug, thanks for guiding me. :)

facebook-github-bot pushed a commit that referenced this pull request Jun 10, 2021
Reviewed By: NicolasHug

Differential Revision: D29027333

fbshipit-source-id: e9a6e49df429c8c0f5604bcf0963fb43907767a3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants