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

Remove p-value checks in test_transforms.py #4756

Merged
merged 9 commits into from
Oct 27, 2021

Conversation

NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Oct 27, 2021

Towards #4506 (comment)

This PR re-works most p_value tests in test_transforms.py.

There are still a few p_value tests in the file, but they're testing functionalities that aren't as simple as just doing if torch.rand(1) < p: return orig_img else ..., so we can't simplify them in the same way

cc @vfdev-5 @datumbox @pmeier

@facebook-github-bot
Copy link

facebook-github-bot commented Oct 27, 2021

💊 CI failures summary and remediations

As of commit efb94f8 (more details on the Dr. CI page):


  • 1/1 failures introduced in this PR

1 failure not recognized by patterns:

Job Step Action
CircleCI binary_linux_conda_py3.9_cu111 packaging/build_conda.sh 🔁 rerun

1 job timed out:

  • binary_linux_conda_py3.9_cu111

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@@ -1362,160 +1361,42 @@ def test_to_grayscale():
trans4.__repr__()


@pytest.mark.skipif(stats is None, reason="scipy.stats not available")
def test_random_grayscale():
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved this one into test_randomness above. It will cover all the randomness-related checks in this test. For the rest (expected values), everything is already covered in test_to_grayscale() just above.

@@ -2011,72 +1893,6 @@ def test_randomperspective_fill(mode):
F.perspective(img_conv, startpoints, endpoints, fill=tuple([fill] * wrong_num_bands))


@pytest.mark.skipif(stats is None, reason="scipy.stats not available")
def test_random_vertical_flip():
Copy link
Member Author

Choose a reason for hiding this comment

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

moved this one in test_randomness above, same for horizontal split.

assert_equal(gray_np, gray_np_3)
@pytest.mark.parametrize("seed", range(10))
@pytest.mark.parametrize("p", (0, 1))
def test_random_apply(p, seed):
Copy link
Member Author

Choose a reason for hiding this comment

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

I could move this one up with test_randomness but this would require taking care of the case where fn is None -- this transform doesn't have a functional equivalent. Not worth it IMHO considering how simple the code is, but I don't mind

Copy link
Contributor

Choose a reason for hiding this comment

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

Including this above will also have the negative side-effect of requiring you to modify your check condition for p=1 to out != img which IMO is less strong to what you have above. I agree keeping this as you have it.

@NicolasHug NicolasHug marked this pull request as ready for review October 27, 2021 11:47
@NicolasHug NicolasHug changed the title WIP remove p-value checks in test_transforms.py Remove p-value checks in test_transforms.py Oct 27, 2021
Copy link
Contributor

@datumbox datumbox 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 for cleaning up the code.

I've only 1 remark for your consideration but it's non blocking.

test/test_transforms.py Outdated Show resolved Hide resolved
assert_equal(gray_np, gray_np_3)
@pytest.mark.parametrize("seed", range(10))
@pytest.mark.parametrize("p", (0, 1))
def test_random_apply(p, seed):
Copy link
Contributor

Choose a reason for hiding this comment

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

Including this above will also have the negative side-effect of requiring you to modify your check condition for p=1 to out != img which IMO is less strong to what you have above. I agree keeping this as you have it.

test/test_transforms.py Outdated Show resolved Hide resolved
@NicolasHug NicolasHug merged commit b621e38 into pytorch:main Oct 27, 2021
@github-actions
Copy link

Hey @NicolasHug!

You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

facebook-github-bot pushed a commit that referenced this pull request Nov 1, 2021
Summary:
* Change test_random_apply

* Change test_random_choice

* Change test_randomness

* took care of RandomVert/HorizFlip

* take care of RandomGrayScale

* minor cleanup

* avoid 0 degree rotation just in case

Reviewed By: datumbox

Differential Revision: D32064690

fbshipit-source-id: 8a67a56fe1e959d144b2cdf49c76610e00933508
cyyever pushed a commit to cyyever/vision that referenced this pull request Nov 16, 2021
* Change test_random_apply

* Change test_random_choice

* Change test_randomness

* took care of RandomVert/HorizFlip

* take care of RandomGrayScale

* minor cleanup

* avoid 0 degree rotation just in case
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