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

Port test/test_transforms.py to pytest #3945

Closed
16 tasks done
NicolasHug opened this issue Jun 2, 2021 · 26 comments · Fixed by #4026
Closed
16 tasks done

Port test/test_transforms.py to pytest #3945

NicolasHug opened this issue Jun 2, 2021 · 26 comments · Fixed by #4026

Comments

@NicolasHug
Copy link
Member

NicolasHug commented Jun 2, 2021

Currently, most tests in test/test_transforms.py rely on unittest.TestCase. Now that we support pytest, we want to remove the use of the unittest module.

For a similar issue see #3956

Instructions

There are many tests in this file, so I bundled them in multiple related groups below. If you're interested in working on this issue, please comment below with "I'm working on <group X>, <group Y>, etc..." so that others don't pick the same tests as you do. Feel free to pick as many groups as you wish, but please don't submit more than 2 groups per PR in order to keep the reviews manageable. Before picking a group, make sure it wasn't picked by another contributor first. Thanks!!

How to port a test to pytest

Porting a test from unittest to pytest is usually fairly straightforward. For a typical example, see https://github.com/pytorch/vision/pull/3907/files:

  • take the test method out of the Tester(unittest.TestCase) class and just declare it as a function
  • Replace @unittest.skipIf with pytest.mark.skipif(cond, reason=...)
  • remove any use of self.assertXYZ.
    • Typically assertEqual(a, b) can be replaced by assert a == b when a and b are pure python objects (scalars, tuples, lists), and otherwise we can rely on assert_equal which is already used in the file.
    • self.assertRaises should be replaced with the pytest.raises(Exp, match=...): context manager, as done in https://github.com/pytorch/vision/pull/3907/files. Same for warnings with pytest.warns
    • self.assertTrue should be replaced with a plain assert
  • When a function uses for loops to tests multiple parameter values, one should usepytest.mark.parametrize instead, as done e.g. in https://github.com/pytorch/vision/pull/3907/files.
  • It may make sense to keep related tests within a single class. For example here, the tests in group A could be grouped into a TestToPILImage class, the tests in group N could be in TestPad, etc. Not all groups need a dedicated class though, it's on a case-by-case basis.

CC @ShrillShrestha and @zhiqwang in case you're interested in this too :)


Groups:

cc @pmeier

@saswatpp
Copy link
Contributor

saswatpp commented Jun 3, 2021

I am working on group P.
I'll try to send PR soon. :)

@ShrillShrestha
Copy link
Contributor

Thanks for mentioning @NicolasHug. I will try working on Group O.

@zhiqwang
Copy link
Contributor

zhiqwang commented Jun 3, 2021

Thanks for mentioning @NicolasHug :) I will try working on Group A.

@AnirudhDagar
Copy link
Contributor

Hi @NicolasHug, I'd like to work on Group I and J.
I'll try to send a PR soon!

@pmeier
Copy link
Collaborator

pmeier commented Jun 3, 2021

@NicolasHug I can help review. Just ping me.

@Dbhasin1
Copy link
Contributor

Dbhasin1 commented Jun 3, 2021

Hi @NicolasHug, I'll try working on Group N

@DevPranjal
Copy link
Contributor

Hey @NicolasHug, I will try working on Group F.

@sahilg06
Copy link
Contributor

sahilg06 commented Jun 3, 2021

Hi @NicolasHug, I will try working on Group K.

@anartify
Copy link
Contributor

anartify commented Jun 3, 2021

Hi @NicolasHug, I will try working on Group E.

@harishsdev
Copy link
Contributor

For b,c,d,g groups i will raise PR

@NicolasHug
Copy link
Member Author

Thanks @harishsdev! please make sure to submit at most 2 groups per PR to keep the review manageable :)

@Ishan-Kumar2
Copy link
Contributor

Hi @NicolasHug, I will try working on Group M.

@sahilg06
Copy link
Contributor

sahilg06 commented Jun 3, 2021

@NicolasHug in function test_rotate_fill in group K . Do I need to parameterize any variable as there are 2 for loops but 1 is dependent on the other. Please guide.

@NicolasHug
Copy link
Member Author

Ah, I wish we could have covariant parametrization! This is a common struggle.
In the case of test_rotate_fill parametrizing only over modes should be good enough!

@NicolasHug
Copy link
Member Author

Seems like most groups have been taken already, thanks everyone for your interest! I've created another similar issue for those who are interested: #3956

@sahilg06
Copy link
Contributor

sahilg06 commented Jun 3, 2021

@NicolasHug but 2nd for loop in function test_rotate_fill requires nums_bands-list which depends upon modes-list. So we have to define modes-list inside the function even if we do parameterization. So is that okay as it is defeating the purpose of parameterization?

@NicolasHug
Copy link
Member Author

Ah I missed that.

If I understand the logic correctly here, we should be able to remove this last for loop and just hardcode wrong_num_bands to num_bands + 1.

Basically, set(nums_bands) - {num_bands} is always 1 when num_bands is 3 and always 3 when num_bands is 1.
What's important is that wrong_num_bands is different from num_bands, I don't think we really care about the exact value.

@harishsdev
Copy link
Contributor

PR Raised for

Group B

test_accimage_crop
test_accimage_pil_to_tensor
test_accimage_resize
test_accimage_to_tensor
Group C

test_affine
test_random_affine

@vivekkumar7089
Copy link
Contributor

Hi, @NicolasHug I'd like to work on Group H.
I'll try to send a PR soon!!

@vivekkumar7089
Copy link
Contributor

Hi, @NicolasHug I'd like to work on Group L.
I'll try to send a PR soon!!

@harishsdev
Copy link
Contributor

Hi all,I am working only on Group G,Group D

please take initiate to raise PR for Group B,Group C

@AnirudhDagar
Copy link
Contributor

@harishsdev no worries, I can send in a PR if you are not working on them anymore.

@Ishan-Kumar2
Copy link
Contributor

Hi @AnirudhDagar are you working on both B and C? I can help with one.

@AnirudhDagar
Copy link
Contributor

Sure @Ishan-Kumar2! I was about to start with B, but let me know the one you prefer. I'll take the other. :))

@Ishan-Kumar2
Copy link
Contributor

Okay @AnirudhDagar you can continue with B, I'll try C.

@NicolasHug
Copy link
Member Author

All done! Thank you so much everyone for your help!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.